| Message ID | 20240213200900.41722-1-rand.sec96@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | dl2k: Fix potential NULL pointer dereference in receive_packet() | expand |
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>
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.
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?
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.
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. */
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(+)