| Message ID | 20240609045419.240265-3-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: treewide: decorate flexible array members with __counted_by() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
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.
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 {
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(-)