[PATCH] arcnet: com90xx: Mark cards and numcards variables as __ro_after_init

[PATCH] arcnet: com90xx: Mark cards and numcards variables as __ro_after_init

[Len Bao]

The 'cards' and 'numcards' variables are initialized only during the
init phase in the 'com90xx_found' function and never changed. So, mark
these as __ro_after_init.

Signed-off-by: Len Bao <len.bao@gmx.us>
---
 drivers/net/arcnet/com90xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/arcnet/com90xx.c b/drivers/net/arcnet/com90xx.c
index b3b287c16..bdcdccbfb 100644
--- a/drivers/net/arcnet/com90xx.c
+++ b/drivers/net/arcnet/com90xx.c
@@ -67,8 +67,8 @@ static void com90xx_copy_from_card(struct net_device *dev, int bufnum,
 
 /* Known ARCnet cards */
 
-static struct net_device *cards[16];
-static int numcards;
+static struct net_device *cards[16] __ro_after_init;
+static int numcards __ro_after_init;
 
 /* Handy defines for ARCnet specific stuff */
 
-- 
2.43.0

Re: [PATCH] arcnet: com90xx: Mark cards and numcards variables as __ro_after_init

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev

Simon says: This is a driver that doesn't seem to have seen any
            real attention for some time. I do wonder if we should
	    leave it be.

	    But as we are here, I'm passing on the AI generated review
	    with some commentary.

---
arcnet: com90xx: Mark cards and numcards variables as __ro_after_init

This commit marks the cards and numcards variables as __ro_after_init since
they are only initialized during the init phase and are never changed
afterward.

> diff --git a/drivers/net/arcnet/com90xx.c b/drivers/net/arcnet/com90xx.c
> index b3b287c165617..bdcdccbfb0017 100644
> --- a/drivers/net/arcnet/com90xx.c
> +++ b/drivers/net/arcnet/com90xx.c
> @@ -67,8 +67,8 @@ static void com90xx_copy_from_card(struct net_device *dev, int bufnum,
>  
>  /* Known ARCnet cards */
>  
> -static struct net_device *cards[16];
> -static int numcards;
> +static struct net_device *cards[16] __ro_after_init;
> +static int numcards __ro_after_init;

Simon says: I believe this is an existing bug rather than a regression.
            But I am concerned that this patch may make it worse.
	    So I'm wondering if it should be fixed at the same time
	    by a series containing:
	    1. a patch to fix this overflow
	    2. this patch (to set __ro_after_init)

Could this change exacerbate an existing regression where we overflow the
cards array?

com90xx_probe() can test up to 32 default I/O ports. For each valid device
found, com90xx_found() unconditionally executes:

drivers/net/arcnet/com90xx.c:com90xx_found() {
    ...
	if (register_netdev(dev))
		goto err_unmap;

	cards[numcards++] = dev;
	return 0;
    ...
}

If more than 16 devices are successfully probed, this writes past the cards
array. Since this patch marks these variables as __ro_after_init, they are
now placed adjacently in memory.

Is it possible that the overflow will now clobber numcards with a pointer
value, leading to an out-of-bounds index on the next iteration?

Simon says: The issue flagged below also appears to be an existing bug
            rather than a regression. And I don't believe it impacts
	    this patch or needs to block progress of it.

Additionally, while reviewing this code, I noticed a second potential
regression in com90xx_found(). Are we registering the IRQ handler before
initializing the function pointers?

drivers/net/arcnet/com90xx.c:com90xx_found() {
    ...
	/* reserve the irq */
	if (request_irq(airq, arcnet_interrupt, 0, "arcnet (90xx)", dev)) {
		arc_printk(D_NORMAL, dev, "Can't get IRQ %d!\n", airq);
		goto err_release_mem;
	}
	dev->irq = airq;

	/* Initialize the rest of the device structure. */
	lp->card_name = "COM90xx";
	lp->hw.command = com90xx_command;
	lp->hw.status = com90xx_status;
    ...
}

If an interrupt triggers immediately after request_irq() completes (e.g., due
to a spurious interrupt), arcnet_interrupt() will execute, evaluate
!netif_running(dev) as true, and invoke lp->hw.status(dev).

Because alloc_arcdev() zero-allocates the private data, lp->hw.status is NULL
at this point. Can this lead to a NULL pointer dereference?