On Wed. 20 janv. 2021 at 07:16, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> On 19.01.21 18:07, Vincent MAILHOL wrote:
> > On Wed. 20 Jan 2021 at 01:25, Vincent Mailhol
> > <mailhol.vincent@xxxxxxxxxx> wrote:
> >>
> >> After calling netif_rx_ni(skb), dereferencing skb is unsafe.
> >> Especially, the canfd_frame cfd which aliases skb memory is accessed
> >> after the netif_rx_ni().
> >>
> >> fixes: a8f820a380a2 ("can: add Virtual CAN Tunnel driver (vxcan)")
> >> Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> >> ---
> >> drivers/net/can/vxcan.c | 6 ++++--
> >> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
> >> index fa47bab510bb..a525ef8d19b0 100644
> >> --- a/drivers/net/can/vxcan.c
> >> +++ b/drivers/net/can/vxcan.c
> >> @@ -39,6 +39,7 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
> >> struct net_device *peer;
> >> struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> >> struct net_device_stats *peerstats, *srcstats = &dev->stats;
> >> + u8 len;
> >>
> >> if (can_dropped_invalid_skb(dev, skb))
> >> return NETDEV_TX_OK;
> >> @@ -61,12 +62,13 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *skb, struct net_device *dev)
> >> skb->dev = peer;
> >> skb->ip_summed = CHECKSUM_UNNECESSARY;
> >>
> >> + u8 len = cfd->len;
> >
> > len = cfd->len;
> > Silly mistake: u8 not needed twice of course...
>
> not tested -> compile tested -> runtime tested
>
> Choose your level!
>
> :-D
I did all the tests on the first patch (can_restart). For the
other two patches, I greped for similar patterns and just test
compile, but I forgot to select the vxcan module thus could not
see the warning.
I take the blame :-)
> >
> >> if (netif_rx_ni(skb) == NET_RX_SUCCESS) {
> >> srcstats->tx_packets++;
> >> - srcstats->tx_bytes += cfd->len;
> >> + srcstats->tx_bytes += len;
> >> peerstats = &peer->stats;
> >> peerstats->rx_packets++;
> >> - peerstats->rx_bytes += cfd->len;
> >> + peerstats->rx_bytes += len;
> >> }
> >>
> >> out_unlock:
> >> --
> >> 2.26.2
> >>
