Thread: Re: [PATCH] firewire: net: fix use after free in fwnet_finish_incoming_packet()
Brought to you by:
aeb,
bencollins
From: Takashi S. <o-t...@sa...> - 2023-06-24 05:06:42
|
Hi, Thanks for the fix. On Fri, Jun 23, 2023 at 01:39:35PM +0800, Zhang Shurong wrote: > The netif_rx() function frees the skb so we can't dereference it to > save the skb->len. > > Signed-off-by: Zhang Shurong <zha...@fo...> > --- > drivers/firewire/net.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c > index 538bd677c254..7a4d1a478e33 100644 > --- a/drivers/firewire/net.c > +++ b/drivers/firewire/net.c > @@ -479,7 +479,7 @@ static int fwnet_finish_incoming_packet(struct net_device *net, > struct sk_buff *skb, u16 source_node_id, > bool is_broadcast, u16 ether_type) > { > - int status; > + int status, len; > > switch (ether_type) { > case ETH_P_ARP: > @@ -533,13 +533,15 @@ static int fwnet_finish_incoming_packet(struct net_device *net, > } > skb->protocol = protocol; > } > + > + len = skb->len; > status = netif_rx(skb); > if (status == NET_RX_DROP) { > net->stats.rx_errors++; > net->stats.rx_dropped++; > } else { > net->stats.rx_packets++; > - net->stats.rx_bytes += skb->len; > + net->stats.rx_bytes += len; > } > > return 0; I'm not good at network subsystem, but as long as reading network core, I think it better not to access members of sk_buff structure after the call of netif_rx() since the ownership is already delegated from firewire-net to the network core to dispatch network protocol. The patch looks good to me. Reviewed-by: Takashi Sakamoto <o-t...@sa...> The most of code in firewire-net comes from old driver (drivers/ieee1394/eth1394.c), thus the most of linux kernel which supports ieee1394/firewire subsystem affects the issue, however the potential issue has never appeared since the skb is released enough later in the different context. I applied the patch to linux-next branch for v6.5-rc1. Thanks Takashi Sakamoto |