* [PATCH net] macsec: avoid use-after-free in macsec_handle_frame()
@ 2020-10-07 8:42 Eric Dumazet
2020-10-07 14:09 ` Paolo Abeni
2020-10-08 20:00 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2020-10-07 8:42 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet, Paolo Abeni
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net] macsec: avoid use-after-free in macsec_handle_frame()
2020-10-07 8:42 [PATCH net] macsec: avoid use-after-free in macsec_handle_frame() Eric Dumazet
@ 2020-10-07 14:09 ` Paolo Abeni
2020-10-07 14:31 ` Eric Dumazet
2020-10-08 20:00 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2020-10-07 14:09 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller; +Cc: netdev, Eric Dumazet
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] macsec: avoid use-after-free in macsec_handle_frame()
2020-10-07 14:09 ` Paolo Abeni
@ 2020-10-07 14:31 ` Eric Dumazet
2020-10-07 14:45 ` Paolo Abeni
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2020-10-07 14:31 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Eric Dumazet, David S . Miller, netdev
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] macsec: avoid use-after-free in macsec_handle_frame()
2020-10-07 14:31 ` Eric Dumazet
@ 2020-10-07 14:45 ` Paolo Abeni
2020-10-07 15:54 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2020-10-07 14:45 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Eric Dumazet, David S . Miller, netdev
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] macsec: avoid use-after-free in macsec_handle_frame()
2020-10-07 14:45 ` Paolo Abeni
@ 2020-10-07 15:54 ` Eric Dumazet
0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2020-10-07 15:54 UTC (permalink / raw)
To: Paolo Abeni, Eric Dumazet; +Cc: Eric Dumazet, David S . Miller, netdev
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 !
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] macsec: avoid use-after-free in macsec_handle_frame()
2020-10-07 8:42 [PATCH net] macsec: avoid use-after-free in macsec_handle_frame() Eric Dumazet
2020-10-07 14:09 ` Paolo Abeni
@ 2020-10-08 20:00 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2020-10-08 20:00 UTC (permalink / raw)
To: Eric Dumazet; +Cc: davem, netdev, edumazet, pabeni
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-08 20:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 8:42 [PATCH net] macsec: avoid use-after-free in macsec_handle_frame() Eric Dumazet
2020-10-07 14:09 ` Paolo Abeni
2020-10-07 14:31 ` Eric Dumazet
2020-10-07 14:45 ` Paolo Abeni
2020-10-07 15:54 ` Eric Dumazet
2020-10-08 20:00 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).