* [PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet()
@ 2024-02-13 20:09 Rand Deeb
2024-02-14 21:56 ` Jacob Keller
2024-02-15 1:02 ` Jakub Kicinski
0 siblings, 2 replies; 5+ messages in thread
From: Rand Deeb @ 2024-02-13 20:09 UTC (permalink / raw
To: David S . Miller, Jakub Kicinski, netdev, linux-kernel
Cc: deeb.rand, lvc-project, voskresenski.stanislav, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet()
2024-02-13 20:09 [PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet() Rand Deeb
@ 2024-02-14 21:56 ` Jacob Keller
2024-02-15 1:02 ` Jakub Kicinski
1 sibling, 0 replies; 5+ messages in thread
From: Jacob Keller @ 2024-02-14 21:56 UTC (permalink / raw
To: Rand Deeb, David S . Miller, Jakub Kicinski, netdev, linux-kernel
Cc: deeb.rand, lvc-project, voskresenski.stanislav
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet()
2024-02-13 20:09 [PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet() Rand Deeb
2024-02-14 21:56 ` Jacob Keller
@ 2024-02-15 1:02 ` Jakub Kicinski
2024-02-15 23:32 ` Rand Deeb
1 sibling, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2024-02-15 1:02 UTC (permalink / raw
To: Rand Deeb
Cc: David S . Miller, netdev, linux-kernel, deeb.rand, lvc-project,
voskresenski.stanislav
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet()
2024-02-15 1:02 ` Jakub Kicinski
@ 2024-02-15 23:32 ` Rand Deeb
2024-02-16 0:18 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Rand Deeb @ 2024-02-15 23:32 UTC (permalink / raw
To: Jakub Kicinski
Cc: David S . Miller, netdev, linux-kernel, deeb.rand, lvc-project,
voskresenski.stanislav
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet()
2024-02-15 23:32 ` Rand Deeb
@ 2024-02-16 0:18 ` Jakub Kicinski
0 siblings, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2024-02-16 0:18 UTC (permalink / raw
To: Rand Deeb
Cc: David S . Miller, netdev, linux-kernel, deeb.rand, lvc-project,
voskresenski.stanislav
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-02-16 0:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-13 20:09 [PATCH] dl2k: Fix potential NULL pointer dereference in receive_packet() Rand Deeb
2024-02-14 21:56 ` Jacob Keller
2024-02-15 1:02 ` Jakub Kicinski
2024-02-15 23:32 ` Rand Deeb
2024-02-16 0:18 ` Jakub Kicinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.