Re: [PATCH 08/14] net: ipa: Annotate struct ipa_power with __counted_by
On 9/22/23 12:28 PM, Kees Cook wrote:
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct ipa_power.
Looks good, thanks.
Reviewed-by: Alex Elder
Note that there is some interaction between struct ipa_power_data
and struct ipa_power (the former is used to initialize the latter).
Both of these contain flexible arrays counted by another field in
the structure. It seems possible that the way these are initialized
might need slight modification to allow the compiler to do its
enforcement; if that's the case, please reach out to me.
-Alex
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Alex Elder
Cc: "David S. Miller"
Cc: Eric Dumazet
Cc: Jakub Kicinski
Cc: Paolo Abeni
Cc: net...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/net/ipa/ipa_power.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
index 0eaa7a7f3343..e223886123ce 100644
--- a/drivers/net/ipa/ipa_power.c
+++ b/drivers/net/ipa/ipa_power.c
@@ -67,7 +67,7 @@ struct ipa_power {
spinlock_t spinlock;/* used with STOPPED/STARTED power flags */
DECLARE_BITMAP(flags, IPA_POWER_FLAG_COUNT);
u32 interconnect_count;
- struct icc_bulk_data interconnect[];
+ struct icc_bulk_data interconnect[] __counted_by(interconnect_count);
};
/* Initialize interconnects required for IPA operation */
Re: [PATCH][next] staging: greybus: Add __counted_by for struct apr_rx_buf and use struct_size()
On 10/9/23 4:52 PM, Gustavo A. R. Silva wrote:
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for
array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
While there, use struct_size() helper, instead of the open-coded
version, to calculate the size for the allocation of the whole
flexible structure, including of course, the flexible-array member.
This code was found with the help of Coccinelle, and audited and
fixed manually.
Looks good to me, and I like the use of struct_size().
Reviewed-by: Alex Elder
Signed-off-by: Gustavo A. R. Silva
---
drivers/staging/greybus/raw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c
index a00978c8e1d2..b9c6eff7cdc1 100644
--- a/drivers/staging/greybus/raw.c
+++ b/drivers/staging/greybus/raw.c
@@ -29,7 +29,7 @@ struct gb_raw {
struct raw_data {
struct list_head entry;
u32 len;
- u8 data[];
+ u8 data[] __counted_by(len);
};
static const struct class raw_class = {
@@ -73,7 +73,7 @@ static int receive_data(struct gb_raw *raw, u32 len, u8 *data)
goto exit;
}
- raw_data = kmalloc(sizeof(*raw_data) + len, GFP_KERNEL);
+ raw_data = kmalloc(struct_size(raw_data, data, len), GFP_KERNEL);
if (!raw_data) {
retval = -ENOMEM;
goto exit;
Re: [PATCH v2] bus: mhi: ep: Use kcalloc() instead of kzalloc()
On 1/28/24 5:27 AM, Erick Archer wrote: This is an effort to get rid of all multiplications from allocation functions in order to prevent integer overflows [1]. Here the multiplication is obviously safe because the "event_rings" member never can have a value greater than 255 (8 bits). This member is set twice using always FIELD_GET: mhi_cntrl->event_rings = FIELD_GET(MHICFG_NER_MASK, regval); mhi_cntrl->event_rings = FIELD_GET(MHICFG_NER_MASK, regval); And the MHICFG_NER_MASK macro defines the 8 bits mask that guarantees a maximum value of 255. However, using kcalloc() is more appropriate [1] and improves readability. This patch has no effect on runtime behavior. Link: https://github.com/KSPP/linux/issues/162 [1] Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1] Reviewed-by: Gustavo A. R. Silva Signed-off-by: Erick Archer --- Changes in v2: - Add more info in the commit message to better explain the change. (Dan Carpenter) - Add the "Reviewed-by:" tag. Looks good. Reviewed-by: Alex Elder Previous versions: v1 - https://lore.kernel.org/linux-hardening/20240120152518.13006-1-erick.arc...@gmx.com/ --- drivers/bus/mhi/ep/main.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c index 65fc1d738bec..8d7a4102bdb7 100644 --- a/drivers/bus/mhi/ep/main.c +++ b/drivers/bus/mhi/ep/main.c @@ -1149,8 +1149,9 @@ int mhi_ep_power_up(struct mhi_ep_cntrl *mhi_cntrl) mhi_ep_mmio_mask_interrupts(mhi_cntrl); mhi_ep_mmio_init(mhi_cntrl); - mhi_cntrl->mhi_event = kzalloc(mhi_cntrl->event_rings * (sizeof(*mhi_cntrl->mhi_event)), - GFP_KERNEL); + mhi_cntrl->mhi_event = kcalloc(mhi_cntrl->event_rings, + sizeof(*mhi_cntrl->mhi_event), + GFP_KERNEL); if (!mhi_cntrl->mhi_event) return -ENOMEM; -- 2.25.1
Re: [PATCH] greybus: Avoid fake flexible array for response data
On 2/16/24 5:28 PM, Kees Cook wrote:
FORTIFY_SOURCE has been ignoring 0-sized destinations while the kernel
code base has been converted to flexible arrays. In order to enforce
the 0-sized destinations (e.g. with __counted_by), the remaining 0-sized
destinations need to be handled. Instead of converting an empty struct
into using a flexible array, just directly use a pointer without any
additional indirection. Remove struct gb_bootrom_get_firmware_response
and struct gb_fw_download_fetch_firmware_response.
The only down side I see is that it sort of disrupts a pattern
used on Greybus request handlers (and the response structure definitions).
I think a one-line comment in place of each of these two
definitions would be helpful, something like:
/* gb_fw_download_fetch_firmware_response contains no data */
And then add a similar comment above the calls to
gb_operation_response_alloc().
Otherwise this looks good.
Reviewed-by: Alex Elder
Signed-off-by: Kees Cook
---
Cc: Viresh Kumar
Cc: Johan Hovold
Cc: Alex Elder
Cc: Greg Kroah-Hartman
Cc: Gustavo A. R. Silva
Cc: greybus-...@lists.linaro.org
Cc: linux-stag...@lists.linux.dev
---
drivers/staging/greybus/bootrom.c | 7 +++
drivers/staging/greybus/fw-download.c | 7 +++
include/linux/greybus/greybus_protocols.h | 8
3 files changed, 6 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c
b/drivers/staging/greybus/bootrom.c
index 79581457c4af..76ad5f289072 100644
--- a/drivers/staging/greybus/bootrom.c
+++ b/drivers/staging/greybus/bootrom.c
@@ -243,10 +243,10 @@ static int gb_bootrom_get_firmware(struct gb_operation
*op)
struct gb_bootrom *bootrom = gb_connection_get_data(op->connection);
const struct firmware *fw;
struct gb_bootrom_get_firmware_request *firmware_request;
- struct gb_bootrom_get_firmware_response *firmware_response;
struct device *dev = &op->connection->bundle->dev;
unsigned int offset, size;
enum next_request_type next_request;
+ u8 *firmware_response;
int ret = 0;
/* Disable timeouts */
@@ -280,15 +280,14 @@ static int gb_bootrom_get_firmware(struct gb_operation
*op)
goto unlock;
}
- if (!gb_operation_response_alloc(op, sizeof(*firmware_response) + size,
-GFP_KERNEL)) {
+ if (!gb_operation_response_alloc(op, size, GFP_KERNEL)) {
dev_err(dev, "%s: error allocating response\n", __func__);
ret = -ENOMEM;
goto unlock;
}
firmware_response = op->response->payload;
- memcpy(firmware_response->data, fw->data + offset, size);
+ memcpy(firmware_response, fw->data + offset, size);
dev_dbg(dev, "responding with firmware (offs = %u, size = %u)\n",
offset, size);
diff --git a/drivers/staging/greybus/fw-download.c
b/drivers/staging/greybus/fw-download.c
index 543692c567f9..2d73bb729aa2 100644
--- a/drivers/staging/greybus/fw-download.c
+++ b/drivers/staging/greybus/fw-download.c
@@ -271,11 +271,11 @@ static int fw_download_fetch_firmware(struct gb_operation
*op)
struct gb_connection *connection = op->connection;
struct fw_download *fw_download = gb_connection_get_data(connection);
struct gb_fw_download_fetch_firmware_request *request;
- struct gb_fw_download_fetch_firmware_response *response;
struct fw_request *fw_req;
const struct firmware *fw;
unsigned int offset, size;
u8 firmware_id;
+ u8 *response;
int ret = 0;
if (op->request->payload_size != sizeof(*request)) {
@@ -325,8 +325,7 @@ static int fw_download_fetch_firmware(struct gb_operation
*op)
goto put_fw;
}
- if (!gb_operation_response_alloc(op, sizeof(*response) + size,
-GFP_KERNEL)) {
+ if (!gb_operation_response_alloc(op, size, GFP_KERNEL)) {
dev_err(fw_download->parent,
"error allocating fetch firmware response\n");
ret = -ENOMEM;
@@ -334,7 +333,7 @@ static int fw_download_fetch_firmware(struct gb_operation
*op)
}
response = op->response->payload;
- memcpy(response->data, fw->data + offset, size);
+ memcpy(response, fw->data + offset, size);
dev_dbg(fw_download->parent,
"responding with firmware (offs = %u, size = %u)\n", offset,
diff --git a/include/linux/greybus/greybus_protocols.h
b/include/linux/greybus/greybus_protocols.h
index aeb8f9243545..603acdcc0c6b 100644
--- a/include/linux/greybus/greybus_protocols.h
+++ b/include/linux/greybus/greybus_protocols.h
@@ -232,10 +232,6 @@ struct gb_fw_download_fetch_firmware_request {
__le32 size;
} __packed;
-struct gb_fw_download_fetc
Re: [PATCH] greybus: audio: apbridgea: Remove flexible array from struct audio_apbridgea_hdr
On 2/17/24 9:47 AM, Erick Archer wrote:
When a struct containing a flexible array is included in another struct,
and there is a member after the struct-with-flex-array, there is a
possibility of memory overlap. These cases must be audited [1]. See:
struct inner {
...
int flex[];
};
struct outer {
...
struct inner header;
int overlap;
...
};
This is the scenario for the "struct audio_apbridgea_hdr" structure
that is included in the following "struct audio_apbridgea_*_request"
structures:
Yeah this was not a very good way to define these header
structures, but I'm glad to hear the flexible array at the
end was never used. I don't know why it was there; maybe
it's an artifact from some other information that got removed.
If the code compiles with your change, it ought to be fine.
(It compiles for me.)
It would be good for Vaibhav or Mark to comment though, maybe
they can provide some context.
struct audio_apbridgea_set_config_request
struct audio_apbridgea_register_cport_request
struct audio_apbridgea_unregister_cport_request
struct audio_apbridgea_set_tx_data_size_request
struct audio_apbridgea_prepare_tx_request
struct audio_apbridgea_start_tx_request
struct audio_apbridgea_stop_tx_request
struct audio_apbridgea_shutdown_tx_request
struct audio_apbridgea_set_rx_data_size_request
struct audio_apbridgea_prepare_rx_request
struct audio_apbridgea_start_rx_request
struct audio_apbridgea_stop_rx_request
struct audio_apbridgea_shutdown_rx_request
The pattern is like the one shown below:
struct audio_apbridgea_hdr {
...
__u8 data[];
} __packed;
struct audio_apbridgea_*_request {
struct audio_apbridgea_hdr hdr;
...
} __packed;
In this case, the trailing flexible array can be removed because it is
never used.
Link: https://github.com/KSPP/linux/issues/202 [1]
Signed-off-by: Erick Archer
---
Hi everyone,
I'm not sure this patch is correct. My concerns are:
The "struct audio_apbridgea_hdr" structure is used as a first member in
all the "struct audio_apbridgea_*_request" structures. And these last
structures are used in the "gb_audio_apbridgea_*(...)" functions. These
functions fill the "request" structure and always use:
gb_hd_output(connection->hd, &req, sizeof(req),
GB_APB_REQUEST_AUDIO_CONTROL, true);
Then, the "gb_hd_output(struct gb_host_device *hd, ...)" function calls:
hd->driver->output(hd, req, size, cmd, async);
The first parameter to this function is of type:
struct gb_host_device {
...
const struct gb_hd_driver *driver;
...
};
And the "gb_hd_driver" structure is defined as:
struct gb_hd_driver {
...
int (*output)(struct gb_host_device *hd, void *req, u16 size,
u8 cmd,
bool async);
};
Therefore, my question is:
Where is the "output" function pointer set? I think I'm missing something.
I think it will be drivers/greybus/es2.c:output().
But in any case, the output function will know nothing about
the structure of the request, so I think it's unrelated to
the change you're proposing.
Johan can confirm this.
I'd like to hear from these others, but otherwise this change
looks good to me.
Reviewed-by: Alex Elder
I have search for another greybus drivers and I have found that, for
example, the "es2_ap_driver" (drivers/greybus/es2.c) sets the "output"
member in:
static struct gb_hd_driver es2_driver = {
...
.output = output,
};
I think that the flexible array that I have removed should be used in
the function assigned to the "output" function pointer. But I am not
able to find this function.
A bit of light on this would be greatly appreciated.
Thanks,
Erick
---
drivers/staging/greybus/audio_apbridgea.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/staging/greybus/audio_apbridgea.h
b/drivers/staging/greybus/audio_apbridgea.h
index efec0f815efd..ab707d310129 100644
--- a/drivers/staging/greybus/audio_apbridgea.h
+++ b/drivers/staging/greybus/audio_apbridgea.h
@@ -65,7 +65,6 @@
struct audio_apbridgea_hdr {
__u8type;
__le16 i2s_port;
- __u8data[];
} __packed;
struct audio_apbridgea_set_config_request {
--
2.25.1
Re: [PATCH] greybus: Avoid fake flexible array for response data
On 2/17/24 3:58 PM, Kees Cook wrote: On Sat, Feb 17, 2024 at 02:17:33PM -0600, Alex Elder wrote: On 2/16/24 5:28 PM, Kees Cook wrote: FORTIFY_SOURCE has been ignoring 0-sized destinations while the kernel code base has been converted to flexible arrays. In order to enforce the 0-sized destinations (e.g. with __counted_by), the remaining 0-sized destinations need to be handled. Instead of converting an empty struct into using a flexible array, just directly use a pointer without any additional indirection. Remove struct gb_bootrom_get_firmware_response and struct gb_fw_download_fetch_firmware_response. The only down side I see is that it sort of disrupts a pattern used on Greybus request handlers (and the response structure definitions). I think a one-line comment in place of each of these two definitions would be helpful, something like: /* gb_fw_download_fetch_firmware_response contains no data */ Er, maybe this should be "no other data" ? Do you want a v2 of this patch? Sending v2 is probably best, because I'd like to see these comments. Greg could fix it up himself but he probably wants to pull it from the list And yes, "no other data" is fine, or maybe "no payload" or "has an empty payload". Any of those is better than nothing; you choose. Thank you. -Alex And then add a similar comment above the calls to gb_operation_response_alloc(). Otherwise this looks good. Reviewed-by: Alex Elder Thanks!
Re: [PATCH v2] greybus: Avoid fake flexible array for response data
On 3/4/24 3:19 PM, Kees Cook wrote:
FORTIFY_SOURCE has been ignoring 0-sized destinations while the kernel
code base has been converted to flexible arrays. In order to enforce
the 0-sized destinations (e.g. with __counted_by), the remaining 0-sized
destinations need to be handled. Instead of converting an empty struct
into using a flexible array, just directly use a pointer without any
additional indirection. Remove struct gb_bootrom_get_firmware_response
and struct gb_fw_download_fetch_firmware_response.
Signed-off-by: Kees Cook
Thanks for adding the comments! This looks good to me.
Reviewed-by: Alex Elder
I want to call attention to a few other spots that should
get a little more attention--related directly to what you're
doing here.
I noticed that the GB_CONTROL_TYPE_GET_MANIFEST response
structure also contains only a flexible array. It might
be good to add a similar comment in gb_interface_enable(),
above this line:
manifest = kmalloc(size, GFP_KERNEL);
The definition of the gb_control_get_manifest_response structure
could probably be replaced with a comment.
The response buffer for an I2C transfer consists only of incoming
data. There is already a comment in gb_i2c_operation_create()
that says this:
/* Response consists only of incoming data */
The definition of the gb_i2c_transfer_response structure should
then go away, in favor of a comment saying this.
The response buffer for a SPI transfer consists only of incoming
data. It is used three times in "driver/staging/greybus/spilib.c":
- calc_rx_xfer_size() subtracts the size of the response structure,
and that should be replaced by a comment (and the structure
definition should go away)
- gb_spi_decode_response() takes the response structure as an
argument. That could be replaced with a void pointer instead,
with a comment.
- gb_spi_transfer_one_message() is what passes the response buffer
to gb_spi_decode_response(), and could be adjusted to reflect
that the response consists only of data--rather than a struct
containing only a flexible array.
Kees: I'm *not* asking you to deal with these, I'm just mentioning
them to you. My comments above (without someone else confirming)
are not sufficient to dictate how to address these.
-Alex
---
Cc: Alex Elder
Cc: Viresh Kumar
Cc: Johan Hovold
Cc: Greg Kroah-Hartman
Cc: Gustavo A. R. Silva
Cc: greybus-...@lists.linaro.org
Cc: linux-stag...@lists.linux.dev
v2: add comments about removed structs
v1:
https://patchwork.kernel.org/project/linux-hardening/patch/20240216232824.work.862-k...@kernel.org/
---
drivers/staging/greybus/bootrom.c | 8
drivers/staging/greybus/fw-download.c | 8
include/linux/greybus/greybus_protocols.h | 8 ++--
3 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c
b/drivers/staging/greybus/bootrom.c
index 79581457c4af..c0d338db6b52 100644
--- a/drivers/staging/greybus/bootrom.c
+++ b/drivers/staging/greybus/bootrom.c
@@ -243,10 +243,10 @@ static int gb_bootrom_get_firmware(struct gb_operation
*op)
struct gb_bootrom *bootrom = gb_connection_get_data(op->connection);
const struct firmware *fw;
struct gb_bootrom_get_firmware_request *firmware_request;
- struct gb_bootrom_get_firmware_response *firmware_response;
struct device *dev = &op->connection->bundle->dev;
unsigned int offset, size;
enum next_request_type next_request;
+ u8 *firmware_response;
int ret = 0;
/* Disable timeouts */
@@ -280,15 +280,15 @@ static int gb_bootrom_get_firmware(struct gb_operation
*op)
goto unlock;
}
- if (!gb_operation_response_alloc(op, sizeof(*firmware_response) + size,
-GFP_KERNEL)) {
+ /* gb_bootrom_get_firmware_response contains only a byte array */
+ if (!gb_operation_response_alloc(op, size, GFP_KERNEL)) {
dev_err(dev, "%s: error allocating response\n", __func__);
ret = -ENOMEM;
goto unlock;
}
firmware_response = op->response->payload;
- memcpy(firmware_response->data, fw->data + offset, size);
+ memcpy(firmware_response, fw->data + offset, size);
dev_dbg(dev, "responding with firmware (offs = %u, size = %u)\n",
offset, size);
diff --git a/drivers/staging/greybus/fw-download.c
b/drivers/staging/greybus/fw-download.c
index 543692c567f9..a06065fb0c5e 100644
--- a/drivers/staging/greybus/fw-download.c
+++ b/drivers/staging/greybus/fw-download.c
@@ -271,11 +271,11 @@ static int fw_download_fetch_firmware(struct gb_operation
*op)
struct gb_connection *connection = op->connection;
struct fw_download *fw_download = gb_connection_get_data(connection)
Re: [PATCH] EISA: Increase length of device names
On 3/10/25 5:24 PM, Kees Cook wrote:
GCC 15's -Wunterminated-string-initialization warned about truncated
name strings. Instead of marking them with the "nonstring" attribute[1],
increase their length to correctly include enough space for the
terminating NUL character, as they are used with %s format specifiers.
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117178 [1]
Cc: Greg Kroah-Hartman
Cc: Azeem Shaikh
Cc: Alex Elder
This was interesting, but based on the bug text I suspect you
meant to address this to Alejandro Colomar, .
For what it's worth, it looks fine to me.
-Alex
Cc: Sumit Garg
Signed-off-by: Kees Cook
---
drivers/eisa/eisa-bus.c | 2 +-
include/linux/eisa.h| 4 +++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/eisa/eisa-bus.c b/drivers/eisa/eisa-bus.c
index cb586a362944..edceea083b98 100644
--- a/drivers/eisa/eisa-bus.c
+++ b/drivers/eisa/eisa-bus.c
@@ -21,7 +21,7 @@
struct eisa_device_info {
struct eisa_device_id id;
- char name[50];
+ char name[EISA_DEVICE_INFO_NAME_SIZE];
};
#ifdef CONFIG_EISA_NAMES
diff --git a/include/linux/eisa.h b/include/linux/eisa.h
index f98200cae637..c73a410bf88f 100644
--- a/include/linux/eisa.h
+++ b/include/linux/eisa.h
@@ -28,6 +28,8 @@
#define EISA_CONFIG_ENABLED 1
#define EISA_CONFIG_FORCED 2
+#define EISA_DEVICE_INFO_NAME_SIZE 51
+
/* There is not much we can say about an EISA device, apart from
* signature, slot number, and base address. dma_mask is set by
* default to parent device mask..*/
@@ -41,7 +43,7 @@ struct eisa_device {
u64 dma_mask;
struct device dev; /* generic device */
#ifdef CONFIG_EISA_NAMES
- char pretty_name[50];
+ char pretty_name[EISA_DEVICE_INFO_NAME_SIZE];
#endif
};
