[PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet()

[PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet()

[Rand Deeb]

This patch addresses a potential NULL pointer dereference issue in the
receive_packet() function of the dl2k network driver. Previously, there was
a possibility of dereferencing a NULL pointer when the pointer 'skb'
returned from the function 'netdev_alloc_skb_ip_align()' was not checked
before being used. To resolve this issue, the patch introduces a check to
ensure that 'skb' is not NULL before dereferencing it.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Rand Deeb <rand.sec96@gmail.com>
---
 drivers/net/ethernet/dlink/dl2k.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/dlink/dl2k.c b/drivers/net/ethernet/dlink/dl2k.c
index 734acb834c98..9cee510247e1 100644
--- a/drivers/net/ethernet/dlink/dl2k.c
+++ b/drivers/net/ethernet/dlink/dl2k.c
@@ -972,6 +972,14 @@ receive_packet (struct net_device *dev)
 							   np->rx_buf_sz,
 							   DMA_FROM_DEVICE);
 			}
+			if (skb == NULL) {
+				np->rx_ring[entry].fraginfo = 0;
+				printk (KERN_INFO
+				       "%s: receive_packet: "
+				       "Unable to re-allocate Rx skbuff.#%d\n",
+				       dev->name, entry);
+				break;
+			}
 			skb->protocol = eth_type_trans (skb, dev);
 #if 0
 			/* Checksum done by hw, but csum value unavailable. */
-- 
2.34.1

Re: [PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet()

[Jacob Keller]

On 2/13/2024 12:09 PM, Rand Deeb wrote:
The subject doesn't indicate which tree for netdev it would target, but
this seems like an obvious bug fix for net.

> @@ -972,6 +972,14 @@ receive_packet (struct net_device *dev)
>  							   np->rx_buf_sz,
>  							   DMA_FROM_DEVICE);
>  			}
> +			if (skb == NULL) {
> +				np->rx_ring[entry].fraginfo = 0;
> +				printk (KERN_INFO
> +				       "%s: receive_packet: "
> +				       "Unable to re-allocate Rx skbuff.#%d\n",
> +				       dev->name, entry);

You could use netdev_warn here instead of printk, which would include
the device information. I checked a couple other drivers and none of
them appear to log any message when this fails. A handful increment a
driver count of how many allocation failures occurred. I'm not sure how
helpful this print would be in practice.

With or without changes to the printed warning message:

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet()

[Jakub Kicinski]

On Tue, 13 Feb 2024 23:09:00 +0300 Rand Deeb wrote:
> +			if (skb == NULL) {

if (!skb) is more common

> +				np->rx_ring[entry].fraginfo = 0;
> +				printk (KERN_INFO
> +				       "%s: receive_packet: "
> +				       "Unable to re-allocate Rx skbuff.#%d\n",
> +				       dev->name, entry);

no prints on allocation failure, please, there logs will include OOM
splats already. A counter as suggested by Jake would be better.
-- 
pw-bot: cr

Re: [PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet()

[Rand Deeb]

On Thu, Feb 15, 2024 at 4:02 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 13 Feb 2024 23:09:00 +0300 Rand Deeb wrote:
> > +                     if (skb == NULL) {
>
> if (!skb) is more common
>
> > +                             np->rx_ring[entry].fraginfo = 0;
> > +                             printk (KERN_INFO
> > +                                    "%s: receive_packet: "
> > +                                    "Unable to re-allocate Rx skbuff.#%d\n",
> > +                                    dev->name, entry);
>
> no prints on allocation failure, please, there logs will include OOM
> splats already. A counter as suggested by Jake would be better.
> --
> pw-bot: cr

Dear Jakub,
Thank you for your feedback and suggestions.

Regarding your comment on using `(!skb)` instead of `(skb == NULL)`, I
understand that `(!skb)` is more common and is also recommended by `
checkpatch.pl`. However, I chose to keep the original code style and logic
to maintain consistency and avoid confusion, especially for other
developers who might be familiar with the existing format. The same
applies to the `printk` statement. In the same function, there is an exact
block of code used; should I fix it too?

Re: [PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet()

[Jakub Kicinski]

On Fri, 16 Feb 2024 02:32:54 +0300 Rand Deeb wrote:
> Regarding your comment on using `(!skb)` instead of `(skb == NULL)`, I
> understand that `(!skb)` is more common and is also recommended by `
> checkpatch.pl`. However, I chose to keep the original code style and logic
> to maintain consistency and avoid confusion, especially for other
> developers who might be familiar with the existing format. The same
> applies to the `printk` statement. In the same function, there is an exact
> block of code used; should I fix it too?

Don't worry about surrounding code if it's written in a clearly
outdated style.

If the style is different intentionally, that's different, but
for old code using more modern style helps bring the code base
forward little by little.