On Sun, Jun 09, 2024 at 01:54:19PM +0900, Vincent Mailhol wrote:
> A new __counted_by() attribute was introduced in [1]. It makes the
> compiler's sanitizer aware of the actual size of a flexible array
> member, allowing for additional runtime checks.
>
> Apply the __counted_by() attribute to the obj flexible array member of
> struct mcp251xfd_rx_ring.
>
> Note that the mcp251xfd_rx_ring.obj member is polymorphic: it can be
> either of:
>
> * an array of struct mcp251xfd_hw_rx_obj_can
> * an array of struct mcp251xfd_hw_rx_obj_canfd
>
> The canfd type was chosen in the declaration by the original author to
> reflect the upper bound. We pursue the same logic here: the sanitizer
> will only see the accurate size of canfd frames. For classical can
> frames, it will see a size bigger than the reality, making the check
> incorrect but silent (false negative).
>
> [1] commit dd06e72e68bc ("Compiler Attributes: Add __counted_by macro")
> Link: https://git.kernel.org/torvalds/c/dd06e72e68bc
>
> CC: Kees Cook <kees@xxxxxxxxxx>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> ---
> drivers/net/can/spi/mcp251xfd/mcp251xfd.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> index 24510b3b8020..b7579fba9457 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> @@ -565,7 +565,7 @@ struct mcp251xfd_rx_ring {
> union mcp251xfd_write_reg_buf uinc_buf;
> union mcp251xfd_write_reg_buf uinc_irq_disable_buf;
> struct spi_transfer uinc_xfer[MCP251XFD_FIFO_DEPTH];
> - struct mcp251xfd_hw_rx_obj_canfd obj[];
> + struct mcp251xfd_hw_rx_obj_canfd obj[] __counted_by(obj_num);
> };
>
> struct __packed mcp251xfd_map_buf_nocrc {
This one seems safe:
rx_ring = kzalloc(sizeof(*rx_ring) + rx_obj_size * rx_obj_num,
GFP_KERNEL);
...
rx_ring->obj_num = rx_obj_num;
But I would like to see the above allocation math replaced with
struct_size() usage:
rx_ring = kzalloc(struct_size(rx_ring, obj, rx_obj_num), GFP_KERNEL);
But that leaves me with a question about the use of rx_obj_size in the
original code. Why is this a variable size? rx_ring has only struct
mcp251xfd_hw_rx_obj_canfd objects in the flexible array...
I suspect that struct mcp251xfd_rx_ring needs to actually be using a
union for its flexible array, but I can't find anything that is casting
struct mcp251xfd_rx_ring::obj to struct mcp251xfd_hw_rx_obj_can.
I'm worried about __counted_by getting used here if the flexible array
isn't actually the right type.
--
Kees Cook
