diff mbox series

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

Message ID 20201007084246.4068317-1-eric.dumazet@gmail.com (mailing list archive)
State Accepted
Commit c7cc9200e9b4a2ac172e990ef1975cd42975dad6
Headers show
Series [net] macsec: avoid use-after-free in macsec_handle_frame() | expand

Commit Message

Eric Dumazet Oct. 7, 2020, 8:42 a.m. UTC 
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(-)

Comments

Paolo Abeni Oct. 7, 2020, 2:09 p.m. UTC | #1 
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
Eric Dumazet Oct. 7, 2020, 2:31 p.m. UTC | #2 
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
>
Paolo Abeni Oct. 7, 2020, 2:45 p.m. UTC | #3 
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>
Eric Dumazet Oct. 7, 2020, 3:54 p.m. UTC | #4 
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 !
patchwork-bot+netdevbpf@kernel.org Oct. 8, 2020, 8 p.m. UTC | #5 
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
diff mbox series

Patch

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++;