Re: [PATCH 2/2] can: mcp251xfd: decorate mcp251xfd_rx_ring.obj with __counted_by()

[Kees Cook <kees@kernel.org>]

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@kernel.org>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  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