1. Patchew
  2. linux
  3. View series

[PATCH] xfrm: xfrm_input: fix a possible memory leak in xfrm_input()

Hangyu Hua posted 1 patch 3 years, 11 months ago 
net/xfrm/xfrm_input.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] xfrm: xfrm_input: fix a possible memory leak in xfrm_input()
Posted by Hangyu Hua 3 years, 11 months ago 
xfrm_input needs to handle skb internally. But skb is not freed When
xo->flags & XFRM_GRO == 0 and decaps == 0.

Fixes: 7785bba299a8 ("esp: Add a software GRO codepath")
Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
---
 net/xfrm/xfrm_input.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 144238a50f3d..6f9576352f30 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -742,7 +742,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			gro_cells_receive(&gro_cells, skb);
 			return err;
 		}
-
+		kfree_skb(skb);
 		return err;
 	}
 
-- 
2.25.1
Re: [PATCH] xfrm: xfrm_input: fix a possible memory leak in xfrm_input()
Posted by Steffen Klassert 3 years, 11 months ago 
On Mon, May 30, 2022 at 06:20:46PM +0800, Hangyu Hua wrote:
> xfrm_input needs to handle skb internally. But skb is not freed When
> xo->flags & XFRM_GRO == 0 and decaps == 0.
> 
> Fixes: 7785bba299a8 ("esp: Add a software GRO codepath")
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> ---
>  net/xfrm/xfrm_input.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index 144238a50f3d..6f9576352f30 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -742,7 +742,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  			gro_cells_receive(&gro_cells, skb);
>  			return err;
>  		}
> -
> +		kfree_skb(skb);
>  		return err;
>  	}

Did you test this? The function behind the 'afinfo->the transport_finish()'
pointer handles this skb and frees it in that case.
Re: [PATCH] xfrm: xfrm_input: fix a possible memory leak in xfrm_input()
Posted by Hangyu Hua 3 years, 11 months ago 
On 2022/5/30 18:37, Steffen Klassert wrote:
> On Mon, May 30, 2022 at 06:20:46PM +0800, Hangyu Hua wrote:
>> xfrm_input needs to handle skb internally. But skb is not freed When
>> xo->flags & XFRM_GRO == 0 and decaps == 0.
>>
>> Fixes: 7785bba299a8 ("esp: Add a software GRO codepath")
>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>> ---
>>   net/xfrm/xfrm_input.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
>> index 144238a50f3d..6f9576352f30 100644
>> --- a/net/xfrm/xfrm_input.c
>> +++ b/net/xfrm/xfrm_input.c
>> @@ -742,7 +742,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>>   			gro_cells_receive(&gro_cells, skb);
>>   			return err;
>>   		}
>> -
>> +		kfree_skb(skb);
>>   		return err;
>>   	}
> 
> Did you test this? The function behind the 'afinfo->the transport_finish()'
> pointer handles this skb and frees it in that case.

int xfrm4_transport_finish(struct sk_buff *skb, int async)
{
	struct xfrm_offload *xo = xfrm_offload(skb);
	struct iphdr *iph = ip_hdr(skb);

	iph->protocol = XFRM_MODE_SKB_CB(skb)->protocol;

#ifndef CONFIG_NETFILTER
	if (!async)
		return -iph->protocol;		<--- [1]
#endif
...
	NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING,
		dev_net(skb->dev), NULL, skb, skb->dev, NULL,
		xfrm4_rcv_encap_finish);	<--- [2]
	return 0;
}

int xfrm6_transport_finish(struct sk_buff *skb, int async)
{
	struct xfrm_offload *xo = xfrm_offload(skb);
	int nhlen = skb->data - skb_network_header(skb);

	skb_network_header(skb)[IP6CB(skb)->nhoff] =
		XFRM_MODE_SKB_CB(skb)->protocol;

#ifndef CONFIG_NETFILTER
	if (!async)
		return 1;			<--- [3]
#endif
...
	NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING,
		dev_net(skb->dev), NULL, skb, skb->dev, NULL,
		xfrm6_transport_finish2);
	return 0;				<--- [4]
}

If transport_finish() return in [1] or [3], there will be a memory leak. 
If it return return in [2] and [4], there will not be a memory leak. It 
look like my patch is incorrect.

How do you think i modify the patch as follows?

    			gro_cells_receive(&gro_cells, skb);
    			return err;
    		}
  -
  +		if (err != 0)
  +			kfree_skb(skb);
    		return err;
    	}
Re: [PATCH] xfrm: xfrm_input: fix a possible memory leak in xfrm_input()
Posted by Steffen Klassert 3 years, 11 months ago 
On Tue, May 31, 2022 at 10:12:05AM +0800, Hangyu Hua wrote:
> On 2022/5/30 18:37, Steffen Klassert wrote:
> > On Mon, May 30, 2022 at 06:20:46PM +0800, Hangyu Hua wrote:
> > > xfrm_input needs to handle skb internally. But skb is not freed When
> > > xo->flags & XFRM_GRO == 0 and decaps == 0.
> > > 
> > > Fixes: 7785bba299a8 ("esp: Add a software GRO codepath")
> > > Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> > > ---
> > >   net/xfrm/xfrm_input.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> > > index 144238a50f3d..6f9576352f30 100644
> > > --- a/net/xfrm/xfrm_input.c
> > > +++ b/net/xfrm/xfrm_input.c
> > > @@ -742,7 +742,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
> > >   			gro_cells_receive(&gro_cells, skb);
> > >   			return err;
> > >   		}
> > > -
> > > +		kfree_skb(skb);
> > >   		return err;
> > >   	}
> > 
> > Did you test this? The function behind the 'afinfo->the transport_finish()'
> > pointer handles this skb and frees it in that case.
> 
> int xfrm4_transport_finish(struct sk_buff *skb, int async)
> {
> 	struct xfrm_offload *xo = xfrm_offload(skb);
> 	struct iphdr *iph = ip_hdr(skb);
> 
> 	iph->protocol = XFRM_MODE_SKB_CB(skb)->protocol;
> 
> #ifndef CONFIG_NETFILTER
> 	if (!async)
> 		return -iph->protocol;		<--- [1]
> #endif
> ...
> 	NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING,
> 		dev_net(skb->dev), NULL, skb, skb->dev, NULL,
> 		xfrm4_rcv_encap_finish);	<--- [2]
> 	return 0;
> }
> 
> int xfrm6_transport_finish(struct sk_buff *skb, int async)
> {
> 	struct xfrm_offload *xo = xfrm_offload(skb);
> 	int nhlen = skb->data - skb_network_header(skb);
> 
> 	skb_network_header(skb)[IP6CB(skb)->nhoff] =
> 		XFRM_MODE_SKB_CB(skb)->protocol;
> 
> #ifndef CONFIG_NETFILTER
> 	if (!async)
> 		return 1;			<--- [3]
> #endif
> ...
> 	NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING,
> 		dev_net(skb->dev), NULL, skb, skb->dev, NULL,
> 		xfrm6_transport_finish2);
> 	return 0;				<--- [4]
> }
> 
> If transport_finish() return in [1] or [3], there will be a memory leak.

No, even in that case there is no memleak. Look for instance at the
IPv4 case, we return -iph->protocol here.
Then look at ip_protocol_deliver_rcu(). If the ipprot->handler (xfrm)
returns a negative value, this is interpreted as the protocol number
and the packet is resubmitted to the next protocol handler.

Please test your patches before you submit them in the future.
Re: [PATCH] xfrm: xfrm_input: fix a possible memory leak in xfrm_input()
Posted by Hangyu Hua 3 years, 11 months ago 
On 2022/5/31 19:35, Steffen Klassert wrote:
> On Tue, May 31, 2022 at 10:12:05AM +0800, Hangyu Hua wrote:
>> On 2022/5/30 18:37, Steffen Klassert wrote:
>>> On Mon, May 30, 2022 at 06:20:46PM +0800, Hangyu Hua wrote:
>>>> xfrm_input needs to handle skb internally. But skb is not freed When
>>>> xo->flags & XFRM_GRO == 0 and decaps == 0.
>>>>
>>>> Fixes: 7785bba299a8 ("esp: Add a software GRO codepath")
>>>> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
>>>> ---
>>>>    net/xfrm/xfrm_input.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
>>>> index 144238a50f3d..6f9576352f30 100644
>>>> --- a/net/xfrm/xfrm_input.c
>>>> +++ b/net/xfrm/xfrm_input.c
>>>> @@ -742,7 +742,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>>>>    			gro_cells_receive(&gro_cells, skb);
>>>>    			return err;
>>>>    		}
>>>> -
>>>> +		kfree_skb(skb);
>>>>    		return err;
>>>>    	}
>>>
>>> Did you test this? The function behind the 'afinfo->the transport_finish()'
>>> pointer handles this skb and frees it in that case.
>>
>> int xfrm4_transport_finish(struct sk_buff *skb, int async)
>> {
>> 	struct xfrm_offload *xo = xfrm_offload(skb);
>> 	struct iphdr *iph = ip_hdr(skb);
>>
>> 	iph->protocol = XFRM_MODE_SKB_CB(skb)->protocol;
>>
>> #ifndef CONFIG_NETFILTER
>> 	if (!async)
>> 		return -iph->protocol;		<--- [1]
>> #endif
>> ...
>> 	NF_HOOK(NFPROTO_IPV4, NF_INET_PRE_ROUTING,
>> 		dev_net(skb->dev), NULL, skb, skb->dev, NULL,
>> 		xfrm4_rcv_encap_finish);	<--- [2]
>> 	return 0;
>> }
>>
>> int xfrm6_transport_finish(struct sk_buff *skb, int async)
>> {
>> 	struct xfrm_offload *xo = xfrm_offload(skb);
>> 	int nhlen = skb->data - skb_network_header(skb);
>>
>> 	skb_network_header(skb)[IP6CB(skb)->nhoff] =
>> 		XFRM_MODE_SKB_CB(skb)->protocol;
>>
>> #ifndef CONFIG_NETFILTER
>> 	if (!async)
>> 		return 1;			<--- [3]
>> #endif
>> ...
>> 	NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING,
>> 		dev_net(skb->dev), NULL, skb, skb->dev, NULL,
>> 		xfrm6_transport_finish2);
>> 	return 0;				<--- [4]
>> }
>>
>> If transport_finish() return in [1] or [3], there will be a memory leak.
> 
> No, even in that case there is no memleak. Look for instance at the
> IPv4 case, we return -iph->protocol here.
> Then look at ip_protocol_deliver_rcu(). If the ipprot->handler (xfrm)
> returns a negative value, this is interpreted as the protocol number
> and the packet is resubmitted to the next protocol handler.
> 
> Please test your patches before you submit them in the future.
Thanks for your explanation. I will be more careful in the future.

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.