[PATCH 0/2] can: treewide: decorate flexible array members with __counted_by()

[PATCH 0/2] can: treewide: decorate flexible array members with __counted_by()

[Vincent Mailhol]

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.

I went through the full can subtree:

  * drivers/net/can
  * include/linux/can
  * include/uapi/linux/can
  * net/can

to try to identify the flexible array members that would benefit from
this attribute.

The observation is there are not so many flexible array member in the
can subtree to begin with.

Within the few flexible array members, only a two can benefit from the
__counted_by() without complex code refactoring, namely:

  * patch 1/2: struct pciefd_board from
    drivers/net/can/peak_canfd/peak_pciefd_main.c

  * patch 2/2: struct mcp251xfd_rx_ring from
    drivers/net/can/spi/mcp251xfd/mcp251xfd.h

Below is an exhaustive list of all the candidates for which
__counted_by() could not be applied to. If something did not appear
here, it just mean that I failed to catch it during my analysis.

  * All the flexible array members which rely on the
    DECLARE_FLEX_ARRAY() helper macro are out of scope for the
    moment. Those will be reconsidered depending on the output of the
    discussion in [2].

  * struct bcm_msg_head from include/uapi/linux/can/bcm.h is a special
    case. The bcm_msg_head.frames member is polymorphic: it can be
    either of:

      * an array of struct can_frame
      * an array of struct canfd_frame

    As of now, it is declared as struct can_frame for historical
    reasons. An idea is to change the type to struct canfd_frame in
    order to reflect the upper bound, in a similar fashion as struct
    mcp251xfd_rx_ring (c.f. second patch from the series). Except
    that this structure is from the uapi, meaning that such a change
    will likely break the userland, making this a bad idea.

  * struct can_skb_priv from include/linux/can/skb.h does not have a
    count member.

  * struct pucan_tx_msg and struct pucan_rx_msg from
    include/linux/can/dev/peak_canfd.h both have a flexible array
    member d, but it is counted by the four most significant bits of
    the channel_dlc member. However __counted_by() does not accept a
    shift logic. Because the layout of this structure is dictated by
    the device API, refactor is impossible here.

  * struct kvaser_usb_net_priv from
    drivers/net/can/usb/kvaser_usb/kvaser_usb.h has the
    active_tx_contexts member but it does not represent an array
    boundary. Under normal conditions, the driver may write beyond
    kvaser_usb_net_priv.tx_contexts[kvaser_usb_net_priv.active_tx_contexts].
    Code refactoring may be considered here.

  * Finally, struct flexcan_mb from
    drivers/net/can/flexcan/flexcan-core.c and struct pciefd_rx_dma
    from drivers/net/can/peak_canfd/peak_pciefd_main.c do not have
    members to indicate the count. Because the layout of these
    structures is dictated by the device API, refactor is impossible
    here.

[1] commit dd06e72e68bc ("Compiler Attributes: Add __counted_by macro")
Link: https://git.kernel.org/torvalds/c/dd06e72e68bc

[2] stddef: Allow attributes to be used when creating flex arrays
Link: https://lore.kernel.org/all/20240213234023.it.219-kees@kernel.org/T/#u

CC: Kees Cook <kees@kernel.org>

Vincent Mailhol (2):
  can: peak_canfd: decorate pciefd_board.can with __counted_by()
  can: mcp251xfd: decorate mcp251xfd_rx_ring.obj with __counted_by()

 drivers/net/can/peak_canfd/peak_pciefd_main.c | 6 ++++--
 drivers/net/can/spi/mcp251xfd/mcp251xfd.h     | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH 1/2] can: peak_canfd: decorate pciefd_board.can with __counted_by()

[Vincent Mailhol]

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.

Move the end of line comments to the previous line to make room and
apply the __counted_by() attribute to the can flexible array member of
struct pciefd_board.

[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/peak_canfd/peak_pciefd_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/peak_canfd/peak_pciefd_main.c b/drivers/net/can/peak_canfd/peak_pciefd_main.c
index 1df3c4b54f03..636102103a88 100644
--- a/drivers/net/can/peak_canfd/peak_pciefd_main.c
+++ b/drivers/net/can/peak_canfd/peak_pciefd_main.c
@@ -190,8 +190,10 @@ struct pciefd_board {
 	void __iomem *reg_base;
 	struct pci_dev *pci_dev;
 	int can_count;
-	spinlock_t cmd_lock;		/* 64-bits cmds must be atomic */
-	struct pciefd_can *can[];	/* array of network devices */
+	/* 64-bits cmds must be atomic */
+	spinlock_t cmd_lock;
+	/* array of network devices */
+	struct pciefd_can *can[] __counted_by(can_count);
 };
 
 /* supported device ids. */
-- 
2.43.0

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

[Vincent Mailhol]

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 {
-- 
2.43.0

Re: [PATCH 1/2] can: peak_canfd: decorate pciefd_board.can with __counted_by()

[Kees Cook]

On Sun, Jun 09, 2024 at 01:54:18PM +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.
> 
> Move the end of line comments to the previous line to make room and
> apply the __counted_by() attribute to the can flexible array member of
> struct pciefd_board.
> 
> [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/peak_canfd/peak_pciefd_main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/peak_canfd/peak_pciefd_main.c b/drivers/net/can/peak_canfd/peak_pciefd_main.c
> index 1df3c4b54f03..636102103a88 100644
> --- a/drivers/net/can/peak_canfd/peak_pciefd_main.c
> +++ b/drivers/net/can/peak_canfd/peak_pciefd_main.c
> @@ -190,8 +190,10 @@ struct pciefd_board {
>  	void __iomem *reg_base;
>  	struct pci_dev *pci_dev;
>  	int can_count;
> -	spinlock_t cmd_lock;		/* 64-bits cmds must be atomic */
> -	struct pciefd_can *can[];	/* array of network devices */
> +	/* 64-bits cmds must be atomic */
> +	spinlock_t cmd_lock;
> +	/* array of network devices */
> +	struct pciefd_can *can[] __counted_by(can_count);
>  };
>  
>  /* supported device ids. */

You'll need to adjust the code logic that manipulates "can_count", as
accesses to "can" will trap when they're seen as out of bounds. For
example:

        pciefd = devm_kzalloc(&pdev->dev, struct_size(pciefd, can, can_count),
                              GFP_KERNEL);
	...
	/* pciefd->can_count is "0" now */


        while (pciefd->can_count < can_count) {
		...
                pciefd_can_probe(pciefd);
		/* which does: */
        		pciefd->can[pciefd->can_count] = priv;	/// HERE
		...
                pciefd->can_count++;
	}

The access at "HERE" above will trap: "can" is believed to have
"can_count" elements (0 on the first time through the loop).
This needs to be adjusted to increment "can_count" first.

Perhaps:


diff --git a/drivers/net/can/peak_canfd/peak_pciefd_main.c b/drivers/net/can/peak_canfd/peak_pciefd_main.c
index 1df3c4b54f03..df8304b2d291 100644
--- a/drivers/net/can/peak_canfd/peak_pciefd_main.c
+++ b/drivers/net/can/peak_canfd/peak_pciefd_main.c
@@ -676,7 +676,8 @@ static int pciefd_can_probe(struct pciefd_board *pciefd)
 	spin_lock_init(&priv->tx_lock);
 
 	/* save the object address in the board structure */
-	pciefd->can[pciefd->can_count] = priv;
+	pciefd->can_count++;
+	pciefd->can[pciefd->can_count - 1] = priv;
 
 	dev_info(&pciefd->pci_dev->dev, "%s at reg_base=0x%p irq=%d\n",
 		 ndev->name, priv->reg_base, ndev->irq);
@@ -800,8 +801,6 @@ static int peak_pciefd_probe(struct pci_dev *pdev,
 		err = pciefd_can_probe(pciefd);
 		if (err)
 			goto err_free_canfd;
-
-		pciefd->can_count++;
 	}
 
 	/* set system timestamps counter in RST mode */


-- 
Kees Cook

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

[Kees Cook]

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