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

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

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

De-referencing skb after call to gro_cells_receive() is not allowed.
We need to fetch skb->len earlier.

Fixes: 5491e7c6b1a9 ("macsec: enable GRO and RPS on macsec devices")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/macsec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 9159846b8b9388644bcf8f136231726d8cf297f2..787ac2c8e74ebef65534291aacb6dc5d9b839b27 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1077,6 +1077,7 @@ static rx_handler_result_t macsec_handle_frame(struct sk_buff **pskb)
 	struct macsec_rx_sa *rx_sa;
 	struct macsec_rxh_data *rxd;
 	struct macsec_dev *macsec;
+	unsigned int len;
 	sci_t sci;
 	u32 hdr_pn;
 	bool cbit;
@@ -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++;
 
-- 
2.28.0.806.g8561365e88-goog

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

[Paolo Abeni]

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 ?!?

Thanks,

Paolo

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

[Eric Dumazet]

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
>

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

[Paolo Abeni]

On Wed, 2020-10-07 at 16:31 +0200, Eric Dumazet wrote:
> 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.

Ah! I completely missed that code path in gro_cells_receive()!
Thank you for pointing that out!

Acked-by: Paolo Abeni <pabeni@redhat.com>

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

[Eric Dumazet]

On 10/7/20 4:45 PM, Paolo Abeni wrote:

> Ah! I completely missed that code path in gro_cells_receive()!
> Thank you for pointing that out!
> 
> Acked-by: Paolo Abeni <pabeni@redhat.com>
> 

No problems, thanks for reviewing !

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Wed,  7 Oct 2020 01:42:46 -0700 you wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> De-referencing skb after call to gro_cells_receive() is not allowed.
> We need to fetch skb->len earlier.
> 
> Fixes: 5491e7c6b1a9 ("macsec: enable GRO and RPS on macsec devices")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Paolo Abeni <pabeni@redhat.com>
> 
> [...]

Here is the summary with links:
  - [net] macsec: avoid use-after-free in macsec_handle_frame()
    https://git.kernel.org/netdev/net/c/c7cc9200e9b4

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html