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 for Android: free password hash cracker in your pocket

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

Message-ID: <Z4k0M6v3Pl8ozDvK@boxer>
Date: Thu, 16 Jan 2025 17:30:43 +0100
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<dan.carpenter@...aro.org>, <yuehaibing@...wei.com>,
	<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,

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.

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.

If that is correct then you should provide a better explanation in your
commit message.

> 
> 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
> 
>