Re: [PATCH net] macsec: avoid use-after-free in macsec_handle_frame()

[Eric Dumazet <edumazet@google.com>]

On Wed, Oct 7, 2020 at 4:09 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hi,
>
> On Wed, 2020-10-07 at 01:42 -0700, Eric Dumazet wrote:
> > @@ -1232,9 +1233,10 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
> >       macsec_rxsc_put(rx_sc);
> >
> >       skb_orphan(skb);
> > +     len = skb->len;
> >       ret = gro_cells_receive(&macsec->gro_cells, skb);
> >       if (ret == NET_RX_SUCCESS)
> > -             count_rx(dev, skb->len);
> > +             count_rx(dev, len);
> >       else
> >               macsec->secy.netdev->stats.rx_dropped++;
>
> I'm sorry I'm low on coffee, but I can't see the race?!? here we are in
> a BH section, and the above code dereference the skb only if it's has
> been enqueued into the gro_cells napi. It could be dequeued/dropped
> only after we leave this section ?!?

We should think of this as an alias for napi_gro_receive(), and not
make any assumptions.
Semantically the skb has been given to another layer.

netif_rx() can absolutely queue the skb to another cpu backlog (RPS,
RFS...), and the other cpu might have consumed the skb right away.

We can avoid these subtle bugs by respecting simple rules ;)

>
> Thanks,
>
> Paolo
>