netdev - RE: [PATCH iwl-next v2] ixgbe: Fix possible skb NULL pointer dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM6PR11MB4610D74ED1F1AB5575AE246EF3E02@DM6PR11MB4610.namprd11.prod.outlook.com>
Date: Thu, 23 Jan 2025 08:19:34 +0000
From: "Kwapulinski, Piotr" <piotr.kwapulinski@...el.com>
To: "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "dan.carpenter@...aro.org"
	<dan.carpenter@...aro.org>, "yuehaibing@...wei.com" <yuehaibing@...wei.com>,
	"Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>
Subject: RE: [PATCH iwl-next v2] ixgbe: Fix possible skb NULL pointer
 dereference

>-----Original Message-----
>From: Fijalkowski, Maciej <maciej.fijalkowski@...el.com> 
>Sent: Monday, January 20, 2025 3:45 PM
>To: Kwapulinski, Piotr <piotr.kwapulinski@...el.com>
>Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; dan.carpenter@...aro.org; yuehaibing@...wei.com; Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>
>Subject: Re: [PATCH iwl-next v2] ixgbe: Fix possible skb NULL pointer dereference
>
>On Fri, Jan 17, 2025 at 04:49:35PM +0100, Piotr Kwapulinski wrote:
>> The commit c824125cbb18 ("ixgbe: Fix passing 0 to ERR_PTR in
>> ixgbe_run_xdp()") stopped utilizing the ERR-like macros for xdp status 
>> encoding. Propagate this logic to the ixgbe_put_rx_buffer().
>> 
>> The commit also relaxed the skb NULL pointer check - caught by Smatch.
>> Restore this check.
>> 
>> Fixes: c824125cbb18 ("ixgbe: Fix passing 0 to ERR_PTR in 
>> ixgbe_run_xdp()")
>> Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
>> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
>> ---
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 7236f20..c682c3d 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -2098,14 +2098,14 @@ static struct ixgbe_rx_buffer 
>> *ixgbe_get_rx_buffer(struct ixgbe_ring *rx_ring,
>>  
>>  static void ixgbe_put_rx_buffer(struct ixgbe_ring *rx_ring,
>>  				struct ixgbe_rx_buffer *rx_buffer,
>> -				struct sk_buff *skb,
>> -				int rx_buffer_pgcnt)
>> +				struct sk_buff *skb, int rx_buffer_pgcnt,
>> +				int xdp_res)
>>  {
>>  	if (ixgbe_can_reuse_rx_page(rx_buffer, rx_buffer_pgcnt)) {
>>  		/* hand second half of page back to the ring */
>>  		ixgbe_reuse_rx_page(rx_ring, rx_buffer);
>>  	} else {
>> -		if (!IS_ERR(skb) && IXGBE_CB(skb)->dma == rx_buffer->dma) {
>> +		if (skb && !xdp_res && IXGBE_CB(skb)->dma == rx_buffer->dma) {
>
>xdp_res check is redundant here. skb ptr will be non-null only for xdp_res == 0. so skb != NULL implies xdp_res == 0.
That was tempting but eventually the ixgbe_run_xdp() handles the error exclusively. I suggest to leave it as it is.
Thanks,
Piotr

>
>If I am not mistaken:D or ixgbe has some code path I missed.
>
>Besides this, thanks for improving commit message!
>
>>  			/* the page has been released from the ring */
>>  			IXGBE_CB(skb)->page_released = true;
>>  		} else {
>> @@ -2415,7 +2415,8 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
>>  			break;
>>  		}
>>  
>> -		ixgbe_put_rx_buffer(rx_ring, rx_buffer, skb, rx_buffer_pgcnt);
>> +		ixgbe_put_rx_buffer(rx_ring, rx_buffer, skb, rx_buffer_pgcnt,
>> +				    xdp_res);
>>  		cleaned_count++;
>>  
>>  		/* place incomplete frames back on ring for completion */
>> --
>> v1 -> v2
>>   Provide extra details in commit message for motivation of this patch.
>> 
>> 2.43.0
>>