netdev - Re: [PATCH net-next 15/18] can: kvaser_usb: Update stats and state even if alloc_can_err

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250110-notorious-kangaroo-from-atlantis-1645f8-mkl@pengutronix.de>
Date: Fri, 10 Jan 2025 14:10:55 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org, 
	linux-can@...r.kernel.org, kernel@...gutronix.de, Jimmy Assarsson <extja@...ser.com>
Subject: Re: [PATCH net-next 15/18] can: kvaser_usb: Update stats and state
 even if alloc_can_err_skb() fails

On 10.01.2025 12:58:03, Simon Horman wrote:
> On Fri, Jan 10, 2025 at 12:04:23PM +0100, Marc Kleine-Budde wrote:
> > From: Jimmy Assarsson <extja@...ser.com>
> > 
> > Ensure statistics, error counters, and CAN state are updated consistently,
> > even when alloc_can_err_skb() fails during state changes or error message
> > frame reception.
> > 
> > Signed-off-by: Jimmy Assarsson <extja@...ser.com>
> > Link: https://patch.msgid.link/20241230142645.128244-1-extja@kvaser.com
> > Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>
> 
> ...
> 
> > diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> 
> ...
> 
> > @@ -1187,11 +1169,18 @@ static void kvaser_usb_leaf_rx_error(const struct kvaser_usb *dev,
> >  		if (priv->can.restart_ms &&
> >  		    old_state == CAN_STATE_BUS_OFF &&
> >  		    new_state < CAN_STATE_BUS_OFF) {
> > -			cf->can_id |= CAN_ERR_RESTARTED;
> > +			if (cf)
> > +				cf->can_id |= CAN_ERR_RESTARTED;
> >  			netif_carrier_on(priv->netdev);
> >  		}
> >  	}
> >  
> > +	if (!skb) {
> > +		stats->rx_dropped++;
> > +		netdev_warn(priv->netdev, "No memory left for err_skb\n");
> > +		return;
> > +	}
> > +
> >  	switch (dev->driver_info->family) {
> >  	case KVASER_LEAF:
> >  		if (es->leaf.error_factor) {
> 
> Hi Jimmy and Marc,
> 
> The next line of this function is:
> 
> 			cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
> 
> Which dereferences cf. However, the check added at the top of
> this hunk assumes that cf may be NULL. This doesn't seem consistent.

The driver allocates the skb with:

	skb = alloc_can_err_skb(priv->netdev, &cf);

Which in turn calls alloc_can_skb(), which finally calls:

        *cf = skb_put_zero(skb, sizeof(struct can_frame));

To put the cf into the skb.

The newly added check "if (!skb)", takes care of skb allocation errors,
so that the de-referencing of "cf" is OK after this point.

regards,
Marc

P.S.:
IIRC smatch stumbled over the same pattern in another driver a while
back. Is there anything we can do about it?

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)