netdev - RE: [PATCH iwl-next] 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] [day] [month] [year] [list]

Message-ID: <DM6PR11MB46104CB6F3B87512417DB71AF31B2@DM6PR11MB4610.namprd11.prod.outlook.com>
Date: Fri, 17 Jan 2025 12:58:18 +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] ixgbe: Fix possible skb NULL pointer dereference

>-----Original Message-----
>From: Fijalkowski, Maciej <maciej.fijalkowski@...el.com> 
>Sent: Thursday, January 16, 2025 5:31 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] ixgbe: Fix possible skb NULL pointer dereference
>
>On Wed, Jan 15, 2025 at 03:59:04PM +0100, Piotr Kwapulinski wrote:
>> Check both skb NULL pointer dereference and error in ixgbe_put_rx_buffer().
>
>Hi Piotr,
Hi Maciej,

>
>is this only theoretical or have you encountered any system panic? If so please include the splat so that reviewers will be able to understand the context of the fix.
No kernel panic or stack trace.

>
>Generally after looking up the commit pointed by fixes tag it seems that we got rid of IS_ERR(skb) logic and forgot to address this part of code.
Right.

>
>If that is correct then you should provide a better explanation in your commit message.
Will provide,
Thank you,
Piotr

>
>> 
>> Fixes: c824125cbb18 ("ixgbe: Fix passing 0 to ERR_PTR in 
>> ixgbe_run_xdp()")
>> 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) {
>>  			/* 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 */
>> --
>> 2.43.0