NACK: [SRU][F/J][PATCH 1/1] can: hi311x: hi3110_can_ist(): fix potential use-after-free

[Massimiliano Pellizzer]

On Wed, 19 Feb 2025 at 02:45, Koichiro Den <koichiro.den at canonical.com> wrote:
>
> On Wed, Feb 12, 2025 at 07:03:13PM GMT, Massimiliano Pellizzer wrote:
> > From: Dario Binacchi <dario.binacchi at amarulasolutions.com>
> >
> > The commit a22bd630cfff ("can: hi311x: do not report txerr and rxerr
> > during bus-off") removed the reporting of rxerr and txerr even in case
> > of correct operation (i. e. not bus-off).
> >
> > The error count information added to the CAN frame after netif_rx() is
> > a potential use after free, since there is no guarantee that the skb
> > is in the same state. It might be freed or reused.
> >
> > Fix the issue by postponing the netif_rx() call in case of txerr and
> > rxerr reporting.
> >
> > Fixes: a22bd630cfff ("can: hi311x: do not report txerr and rxerr during bus-off")
> > Signed-off-by: Dario Binacchi <dario.binacchi at amarulasolutions.com>
> > Link: https://patch.msgid.link/20241122221650.633981-5-dario.binacchi@amarulasolutions.com
> > Signed-off-by: Marc Kleine-Budde <mkl at pengutronix.de>
> > (backported from commit 9ad86d377ef4a19c75a9c639964879a5b25a433b)
> > [mpellizzer: backported considering differences in context due to
> > missing commit:
> > - 3e5c291c7942d can: add CAN_ERR_CNT flag to notify availability of error counter
> > which do not affect the patch]
> > CVE-2024-56651
> > Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer at canonical.com>
> > ---
> >  drivers/net/can/spi/hi311x.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
> > index 28273e84171a2..660c4fe4614f5 100644
> > --- a/drivers/net/can/spi/hi311x.c
> > +++ b/drivers/net/can/spi/hi311x.c
> > @@ -673,9 +673,9 @@ static irqreturn_t hi3110_can_ist(int irq, void *dev_id)
> >                       tx_state = txerr >= rxerr ? new_state : 0;
> >                       rx_state = txerr <= rxerr ? new_state : 0;
> >                       can_change_state(net, cf, tx_state, rx_state);
> > -                     netif_rx_ni(skb);
> >
> >                       if (new_state == CAN_STATE_BUS_OFF) {
> > +                             netif_rx(skb);
>
> J/F is quite old and netif_rx_ni() is still alive. This handler runs in an
> irq handler thread, so it should be kept as-is. Even if that weren't the
> case, making a silent change should be avoided.

You are totally right. Thanks for catching this and for reviewing.
Will send a v2 soon.

>
> >                               can_bus_off(net);
> >                               if (priv->can.restart_ms == 0) {
> >                                       priv->force_quit = 1;
> > @@ -685,6 +685,7 @@ static irqreturn_t hi3110_can_ist(int irq, void *dev_id)
> >                       } else {
> >                               cf->data[6] = txerr;
> >                               cf->data[7] = rxerr;
> > +                             netif_rx(skb);
> >                       }
> >               }
> >
> > --
> > 2.43.0
> >
> >
> > --
> > kernel-team mailing list
> > kernel-team at lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team

-- 
Massimiliano Pellizzer