diff mbox series

[6.1.y] net: napi: Prevent overflow of napi_defer_hard_irqs

Message ID 20241211040304.3212711-1-jianqi.ren.cn@windriver.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [6.1.y] net: napi: Prevent overflow of napi_defer_hard_irqs | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply, async

Commit Message

Ren, Jianqi (Jacky) (CN) Dec. 11, 2024, 4:03 a.m. UTC 
From: Joe Damato <jdamato@fastly.com>

[ Upstream commit 08062af0a52107a243f7608fd972edb54ca5b7f8 ]

In commit 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
napi_defer_irqs was added to net_device and napi_defer_irqs_count was
added to napi_struct, both as type int.

This value never goes below zero, so there is not reason for it to be a
signed int. Change the type for both from int to u32, and add an
overflow check to sysfs to limit the value to S32_MAX.

The limit of S32_MAX was chosen because the practical limit before this
patch was S32_MAX (anything larger was an overflow) and thus there are
no behavioral changes introduced. If the extra bit is needed in the
future, the limit can be raised.

Before this patch:

$ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
$ cat /sys/class/net/eth4/napi_defer_hard_irqs
-2147483647

After this patch:

$ sudo bash -c 'echo 2147483649 > /sys/class/net/eth4/napi_defer_hard_irqs'
bash: line 0: echo: write error: Numerical result out of range

Similarly, /sys/class/net/XXXXX/tx_queue_len is defined as unsigned:

include/linux/netdevice.h:      unsigned int            tx_queue_len;

And has an overflow check:

dev_change_tx_queue_len(..., unsigned long new_len):

  if (new_len != (unsigned int)new_len)
          return -ERANGE;

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Joe Damato <jdamato@fastly.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Link: https://patch.msgid.link/20240904153431.307932-1-jdamato@fastly.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Jianqi Ren <jianqi.ren.cn@windriver.com>
---
 include/linux/netdevice.h | 4 ++--
 net/core/net-sysfs.c      | 6 +++++-
 2 files changed, 7 insertions(+), 3 deletions(-)

Comments

Jakub Kicinski Dec. 12, 2024, 4:07 a.m. UTC | #1 
On Wed, 11 Dec 2024 12:03:04 +0800 jianqi.ren.cn@windriver.com wrote:
> From: Joe Damato <jdamato@fastly.com>
> 
> [ Upstream commit 08062af0a52107a243f7608fd972edb54ca5b7f8 ]
> 
> In commit 6f8b12d661d0 ("net: napi: add hard irqs deferral feature")
> napi_defer_irqs was added to net_device and napi_defer_irqs_count was
> added to napi_struct, both as type int.
> 
> This value never goes below zero, so there is not reason for it to be a
> signed int. Change the type for both from int to u32, and add an
> overflow check to sysfs to limit the value to S32_MAX.

Could you explain why you want to backport this change to stable?
Ren, Jianqi (Jacky) (CN) Dec. 12, 2024, 7:06 a.m. UTC | #2 
I port the fix to fix CVE-2024-50018 in linux 6.1.

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org> 
Sent: Thursday, December 12, 2024 12:08
To: Ren, Jianqi (Jacky) (CN) <Jianqi.Ren.CN@windriver.com>
Cc: gregkh@linuxfoundation.org; stable@vger.kernel.org; davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; sashal@kernel.org; jamie.bainbridge@gmail.com; jdamato@fastly.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6.1.y] net: napi: Prevent overflow of napi_defer_hard_irqs

CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.

On Wed, 11 Dec 2024 12:03:04 +0800 jianqi.ren.cn@windriver.com wrote:
> From: Joe Damato <jdamato@fastly.com>
>
> [ Upstream commit 08062af0a52107a243f7608fd972edb54ca5b7f8 ]
>
> In commit 6f8b12d661d0 ("net: napi: add hard irqs deferral feature") 
> napi_defer_irqs was added to net_device and napi_defer_irqs_count was 
> added to napi_struct, both as type int.
>
> This value never goes below zero, so there is not reason for it to be 
> a signed int. Change the type for both from int to u32, and add an 
> overflow check to sysfs to limit the value to S32_MAX.

Could you explain why you want to backport this change to stable?
Greg KH Dec. 12, 2024, 11:41 a.m. UTC | #3 
On Wed, Dec 11, 2024 at 12:03:04PM +0800, jianqi.ren.cn@windriver.com wrote:
> From: Joe Damato <jdamato@fastly.com>
> 
> [ Upstream commit 08062af0a52107a243f7608fd972edb54ca5b7f8 ]

You can't ignore the 6.6.y tree :(

Dropping from my review queue now.

greg k-h
Jakub Kicinski Dec. 12, 2024, 2:50 p.m. UTC | #4 
On Thu, 12 Dec 2024 12:41:08 +0100 Greg KH wrote:
> On Wed, Dec 11, 2024 at 12:03:04PM +0800, jianqi.ren.cn@windriver.com wrote:
> > From: Joe Damato <jdamato@fastly.com>
> > 
> > [ Upstream commit 08062af0a52107a243f7608fd972edb54ca5b7f8 ]  
> 
> You can't ignore the 6.6.y tree :(
> 
> Dropping from my review queue now.

Is it possible to instead mark CVE-2024-50018 as invalid, please?
The change is cosmetic.
Greg KH Dec. 12, 2024, 3:17 p.m. UTC | #5 
On Thu, Dec 12, 2024 at 06:50:44AM -0800, Jakub Kicinski wrote:
> On Thu, 12 Dec 2024 12:41:08 +0100 Greg KH wrote:
> > On Wed, Dec 11, 2024 at 12:03:04PM +0800, jianqi.ren.cn@windriver.com wrote:
> > > From: Joe Damato <jdamato@fastly.com>
> > > 
> > > [ Upstream commit 08062af0a52107a243f7608fd972edb54ca5b7f8 ]  
> > 
> > You can't ignore the 6.6.y tree :(
> > 
> > Dropping from my review queue now.
> 
> Is it possible to instead mark CVE-2024-50018 as invalid, please?
> The change is cosmetic.

Now rejected, sorry about that.

greg k-h
diff mbox series

Patch

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index fbbd0df1106b..8379e938cd89 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -352,7 +352,7 @@  struct napi_struct {
 
 	unsigned long		state;
 	int			weight;
-	int			defer_hard_irqs_count;
+	u32			defer_hard_irqs_count;
 	unsigned long		gro_bitmask;
 	int			(*poll)(struct napi_struct *, int);
 #ifdef CONFIG_NETPOLL
@@ -2193,7 +2193,7 @@  struct net_device {
 
 	struct bpf_prog __rcu	*xdp_prog;
 	unsigned long		gro_flush_timeout;
-	int			napi_defer_hard_irqs;
+	u32			napi_defer_hard_irqs;
 #define GRO_LEGACY_MAX_SIZE	65536u
 /* TCP minimal MSS is 8 (TCP_MIN_GSO_SIZE),
  * and shinfo->gso_segs is a 16bit field.
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 8a06f97320e0..4ce57e75d139 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -30,6 +30,7 @@ 
 #ifdef CONFIG_SYSFS
 static const char fmt_hex[] = "%#x\n";
 static const char fmt_dec[] = "%d\n";
+static const char fmt_uint[] = "%u\n";
 static const char fmt_ulong[] = "%lu\n";
 static const char fmt_u64[] = "%llu\n";
 
@@ -405,6 +406,9 @@  NETDEVICE_SHOW_RW(gro_flush_timeout, fmt_ulong);
 
 static int change_napi_defer_hard_irqs(struct net_device *dev, unsigned long val)
 {
+	if (val > S32_MAX)
+		return -ERANGE;
+
 	WRITE_ONCE(dev->napi_defer_hard_irqs, val);
 	return 0;
 }
@@ -418,7 +422,7 @@  static ssize_t napi_defer_hard_irqs_store(struct device *dev,
 
 	return netdev_store(dev, attr, buf, len, change_napi_defer_hard_irqs);
 }
-NETDEVICE_SHOW_RW(napi_defer_hard_irqs, fmt_dec);
+NETDEVICE_SHOW_RW(napi_defer_hard_irqs, fmt_uint);
 
 static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr,
 			     const char *buf, size_t len)