LinuxLists.cc - [PATCH] net: usb: lan78xx: Limit packet length to skb->len

2023-03-17 15:34:02

Subject: [PATCH] net: usb: lan78xx: Limit packet length to skb->len

Packet length retrieved from descriptor may be larger than
the actual socket buffer length. In such case the cloned
skb passed up the network stack will leak kernel memory contents.

Additionally prevent integer underflow when size is less than
ETH_FCS_LEN.

Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
Signed-off-by: Szymon Heidrich <[email protected]>
---
drivers/net/usb/lan78xx.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 068488890..e7d27be84 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -3579,10 +3579,24 @@ static int lan78xx_rx(struct lan78xx_net *dev, struct sk_buff *skb,
size = (rx_cmd_a & RX_CMD_A_LEN_MASK_);
align_count = (4 - ((size + RXW_PADDING) % 4)) % 4;

+ if (unlikely(size > skb->len)) {
+ netif_dbg(dev, rx_err, dev->net,
+ "size err rx_cmd_a=0x%08x\n",
+ rx_cmd_a);
+ return 0;
+ }
+
if (unlikely(rx_cmd_a & RX_CMD_A_RED_)) {
netif_dbg(dev, rx_err, dev->net,
"Error rx_cmd_a=0x%08x", rx_cmd_a);
} else {
+ if (unlikely(size < ETH_FCS_LEN)) {
+ netif_dbg(dev, rx_err, dev->net,
+ "size err rx_cmd_a=0x%08x\n",
+ rx_cmd_a);
+ return 0;
+ }
+
u32 frame_len = size - ETH_FCS_LEN;
struct sk_buff *skb2;

--
2.40.0

2023-03-17 16:47:08

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] net: usb: lan78xx: Limit packet length to skb->len

2023-03-17 17:36:46

by Szymon Heidrich

[permalink] [raw]

Subject: [PATCH v2] net: usb: lan78xx: Limit packet length to skb->len

Packet length retrieved from descriptor may be larger than
the actual socket buffer length. In such case the cloned
skb passed up the network stack will leak kernel memory contents.

Additionally prevent integer underflow when size is less than
ETH_FCS_LEN.

Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
Signed-off-by: Szymon Heidrich <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
V1 -> V2: Fix ISO C90 forbids mixed declarations and code

drivers/net/usb/lan78xx.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 068488890..a150711a1 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -3579,11 +3579,27 @@ static int lan78xx_rx(struct lan78xx_net *dev, struct sk_buff *skb,
size = (rx_cmd_a & RX_CMD_A_LEN_MASK_);
align_count = (4 - ((size + RXW_PADDING) % 4)) % 4;

+ if (unlikely(size > skb->len)) {
+ netif_dbg(dev, rx_err, dev->net,
+ "size err rx_cmd_a=0x%08x\n",
+ rx_cmd_a);
+ return 0;
+ }
+
if (unlikely(rx_cmd_a & RX_CMD_A_RED_)) {
netif_dbg(dev, rx_err, dev->net,
"Error rx_cmd_a=0x%08x", rx_cmd_a);
} else {
- u32 frame_len = size - ETH_FCS_LEN;
+ u32 frame_len;
+
+ if (unlikely(size < ETH_FCS_LEN)) {
+ netif_dbg(dev, rx_err, dev->net,
+ "size err rx_cmd_a=0x%08x\n",
+ rx_cmd_a);
+ return 0;
+ }
+
+ frame_len = size - ETH_FCS_LEN;
struct sk_buff *skb2;

skb2 = napi_alloc_skb(&dev->napi, frame_len);
--
2.40.0

2023-03-18 03:32:44

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2] net: usb: lan78xx: Limit packet length to skb->len

2023-03-18 05:27:09

by Jakub Kicinski

[permalink] [raw]

Subject: Re: [PATCH v2] net: usb: lan78xx: Limit packet length to skb->len

On Fri, 17 Mar 2023 18:36:06 +0100 Szymon Heidrich wrote:
> Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
> Signed-off-by: Szymon Heidrich <[email protected]>
> Reported-by: kernel test robot <[email protected]>

I'd drop the Reported-by: tag in this case, since it makes it look like
the bot reported the bug you're fixing, rather than merely reported a
build issue.

When you repost please avoid posting in reply to previous version.
Makes the review queue easier to organize for mentainers.

2023-03-18 07:10:02

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2] net: usb: lan78xx: Limit packet length to skb->len

Hi Szymon,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/main]
[also build test WARNING on net/main linus/master v6.3-rc2 next-20230317]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Szymon-Heidrich/net-usb-lan78xx-Limit-packet-length-to-skb-len/20230318-013659
patch link: https://lore.kernel.org/r/20230317173606.91426-1-szymon.heidrich%40gmail.com
patch subject: [PATCH v2] net: usb: lan78xx: Limit packet length to skb->len
config: s390-randconfig-r003-20230318 (https://download.01.org/0day-ci/archive/20230318/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/17c060ef752f4a1366ed7d8e62ba5f64f654eced
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Szymon-Heidrich/net-usb-lan78xx-Limit-packet-length-to-skb-len/20230318-013659
git checkout 17c060ef752f4a1366ed7d8e62ba5f64f654eced
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/net/usb/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

In file included from drivers/net/usb/lan78xx.c:6:
In file included from include/linux/netdevice.h:38:
In file included from include/net/net_namespace.h:43:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from drivers/net/usb/lan78xx.c:6:
In file included from include/linux/netdevice.h:38:
In file included from include/net/net_namespace.h:43:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from drivers/net/usb/lan78xx.c:6:
In file included from include/linux/netdevice.h:38:
In file included from include/net/net_namespace.h:43:
In file included from include/linux/skbuff.h:28:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
>> drivers/net/usb/lan78xx.c:3603:20: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
struct sk_buff *skb2;
^
13 warnings generated.

vim +3603 drivers/net/usb/lan78xx.c

55d7de9de6c30a [email protected] 2015-07-30 3552
ec4c7e12396b1a John Efstathiades 2021-11-18 3553 static int lan78xx_rx(struct lan78xx_net *dev, struct sk_buff *skb,
ec4c7e12396b1a John Efstathiades 2021-11-18 3554 int budget, int *work_done)
55d7de9de6c30a [email protected] 2015-07-30 3555 {
0dd87266c1337d John Efstathiades 2021-11-18 3556 if (skb->len < RX_SKB_MIN_LEN)
55d7de9de6c30a [email protected] 2015-07-30 3557 return 0;
55d7de9de6c30a [email protected] 2015-07-30 3558
ec4c7e12396b1a John Efstathiades 2021-11-18 3559 /* Extract frames from the URB buffer and pass each one to
ec4c7e12396b1a John Efstathiades 2021-11-18 3560 * the stack in a new NAPI SKB.
ec4c7e12396b1a John Efstathiades 2021-11-18 3561 */
55d7de9de6c30a [email protected] 2015-07-30 3562 while (skb->len > 0) {
55d7de9de6c30a [email protected] 2015-07-30 3563 u32 rx_cmd_a, rx_cmd_b, align_count, size;
55d7de9de6c30a [email protected] 2015-07-30 3564 u16 rx_cmd_c;
55d7de9de6c30a [email protected] 2015-07-30 3565 unsigned char *packet;
55d7de9de6c30a [email protected] 2015-07-30 3566
bb448f8a60ea93 Chuhong Yuan 2019-07-19 3567 rx_cmd_a = get_unaligned_le32(skb->data);
55d7de9de6c30a [email protected] 2015-07-30 3568 skb_pull(skb, sizeof(rx_cmd_a));
55d7de9de6c30a [email protected] 2015-07-30 3569
bb448f8a60ea93 Chuhong Yuan 2019-07-19 3570 rx_cmd_b = get_unaligned_le32(skb->data);
55d7de9de6c30a [email protected] 2015-07-30 3571 skb_pull(skb, sizeof(rx_cmd_b));
55d7de9de6c30a [email protected] 2015-07-30 3572
bb448f8a60ea93 Chuhong Yuan 2019-07-19 3573 rx_cmd_c = get_unaligned_le16(skb->data);
55d7de9de6c30a [email protected] 2015-07-30 3574 skb_pull(skb, sizeof(rx_cmd_c));
55d7de9de6c30a [email protected] 2015-07-30 3575
55d7de9de6c30a [email protected] 2015-07-30 3576 packet = skb->data;
55d7de9de6c30a [email protected] 2015-07-30 3577
55d7de9de6c30a [email protected] 2015-07-30 3578 /* get the packet length */
55d7de9de6c30a [email protected] 2015-07-30 3579 size = (rx_cmd_a & RX_CMD_A_LEN_MASK_);
55d7de9de6c30a [email protected] 2015-07-30 3580 align_count = (4 - ((size + RXW_PADDING) % 4)) % 4;
55d7de9de6c30a [email protected] 2015-07-30 3581
17c060ef752f4a Szymon Heidrich 2023-03-17 3582 if (unlikely(size > skb->len)) {
17c060ef752f4a Szymon Heidrich 2023-03-17 3583 netif_dbg(dev, rx_err, dev->net,
17c060ef752f4a Szymon Heidrich 2023-03-17 3584 "size err rx_cmd_a=0x%08x\n",
17c060ef752f4a Szymon Heidrich 2023-03-17 3585 rx_cmd_a);
17c060ef752f4a Szymon Heidrich 2023-03-17 3586 return 0;
17c060ef752f4a Szymon Heidrich 2023-03-17 3587 }
17c060ef752f4a Szymon Heidrich 2023-03-17 3588
55d7de9de6c30a [email protected] 2015-07-30 3589 if (unlikely(rx_cmd_a & RX_CMD_A_RED_)) {
55d7de9de6c30a [email protected] 2015-07-30 3590 netif_dbg(dev, rx_err, dev->net,
55d7de9de6c30a [email protected] 2015-07-30 3591 "Error rx_cmd_a=0x%08x", rx_cmd_a);
55d7de9de6c30a [email protected] 2015-07-30 3592 } else {
17c060ef752f4a Szymon Heidrich 2023-03-17 3593 u32 frame_len;
17c060ef752f4a Szymon Heidrich 2023-03-17 3594
17c060ef752f4a Szymon Heidrich 2023-03-17 3595 if (unlikely(size < ETH_FCS_LEN)) {
17c060ef752f4a Szymon Heidrich 2023-03-17 3596 netif_dbg(dev, rx_err, dev->net,
17c060ef752f4a Szymon Heidrich 2023-03-17 3597 "size err rx_cmd_a=0x%08x\n",
17c060ef752f4a Szymon Heidrich 2023-03-17 3598 rx_cmd_a);
17c060ef752f4a Szymon Heidrich 2023-03-17 3599 return 0;
17c060ef752f4a Szymon Heidrich 2023-03-17 3600 }
17c060ef752f4a Szymon Heidrich 2023-03-17 3601
17c060ef752f4a Szymon Heidrich 2023-03-17 3602 frame_len = size - ETH_FCS_LEN;
ec4c7e12396b1a John Efstathiades 2021-11-18 @3603 struct sk_buff *skb2;
55d7de9de6c30a [email protected] 2015-07-30 3604
ec4c7e12396b1a John Efstathiades 2021-11-18 3605 skb2 = napi_alloc_skb(&dev->napi, frame_len);
ec4c7e12396b1a John Efstathiades 2021-11-18 3606 if (!skb2)
55d7de9de6c30a [email protected] 2015-07-30 3607 return 0;
55d7de9de6c30a [email protected] 2015-07-30 3608
ec4c7e12396b1a John Efstathiades 2021-11-18 3609 memcpy(skb2->data, packet, frame_len);
ec4c7e12396b1a John Efstathiades 2021-11-18 3610
ec4c7e12396b1a John Efstathiades 2021-11-18 3611 skb_put(skb2, frame_len);
55d7de9de6c30a [email protected] 2015-07-30 3612
55d7de9de6c30a [email protected] 2015-07-30 3613 lan78xx_rx_csum_offload(dev, skb2, rx_cmd_a, rx_cmd_b);
ec21ecf0aad279 Dave Stevenson 2018-06-25 3614 lan78xx_rx_vlan_offload(dev, skb2, rx_cmd_a, rx_cmd_b);
55d7de9de6c30a [email protected] 2015-07-30 3615
ec4c7e12396b1a John Efstathiades 2021-11-18 3616 /* Processing of the URB buffer must complete once
ec4c7e12396b1a John Efstathiades 2021-11-18 3617 * it has started. If the NAPI work budget is exhausted
ec4c7e12396b1a John Efstathiades 2021-11-18 3618 * while frames remain they are added to the overflow
ec4c7e12396b1a John Efstathiades 2021-11-18 3619 * queue for delivery in the next NAPI polling cycle.
ec4c7e12396b1a John Efstathiades 2021-11-18 3620 */
ec4c7e12396b1a John Efstathiades 2021-11-18 3621 if (*work_done < budget) {
55d7de9de6c30a [email protected] 2015-07-30 3622 lan78xx_skb_return(dev, skb2);
ec4c7e12396b1a John Efstathiades 2021-11-18 3623 ++(*work_done);
ec4c7e12396b1a John Efstathiades 2021-11-18 3624 } else {
ec4c7e12396b1a John Efstathiades 2021-11-18 3625 skb_queue_tail(&dev->rxq_overflow, skb2);
ec4c7e12396b1a John Efstathiades 2021-11-18 3626 }
55d7de9de6c30a [email protected] 2015-07-30 3627 }
55d7de9de6c30a [email protected] 2015-07-30 3628
55d7de9de6c30a [email protected] 2015-07-30 3629 skb_pull(skb, size);
55d7de9de6c30a [email protected] 2015-07-30 3630
ec4c7e12396b1a John Efstathiades 2021-11-18 3631 /* skip padding bytes before the next frame starts */
55d7de9de6c30a [email protected] 2015-07-30 3632 if (skb->len)
55d7de9de6c30a [email protected] 2015-07-30 3633 skb_pull(skb, align_count);
55d7de9de6c30a [email protected] 2015-07-30 3634 }
55d7de9de6c30a [email protected] 2015-07-30 3635
55d7de9de6c30a [email protected] 2015-07-30 3636 return 1;
55d7de9de6c30a [email protected] 2015-07-30 3637 }
55d7de9de6c30a [email protected] 2015-07-30 3638

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

2023-03-23 16:36:25

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v2] net: usb: lan78xx: Limit packet length to skb->len

On Fri, Mar 17, 2023 at 06:36:06PM +0100, Szymon Heidrich wrote:
> Packet length retrieved from descriptor may be larger than
> the actual socket buffer length. In such case the cloned
> skb passed up the network stack will leak kernel memory contents.
>
> Additionally prevent integer underflow when size is less than
> ETH_FCS_LEN.
>
> Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
> Signed-off-by: Szymon Heidrich <[email protected]>
> Reported-by: kernel test robot <[email protected]>

the test robot did not report the fact that the packet length needed to
be limited :(

2023-03-23 21:04:18

by Szymon Heidrich

[permalink] [raw]

Subject: Re: [PATCH v2] net: usb: lan78xx: Limit packet length to skb->len

On 23/03/2023 17:29, Greg KH wrote:
> On Fri, Mar 17, 2023 at 06:36:06PM +0100, Szymon Heidrich wrote:
>> Packet length retrieved from descriptor may be larger than
>> the actual socket buffer length. In such case the cloned
>> skb passed up the network stack will leak kernel memory contents.
>>
>> Additionally prevent integer underflow when size is less than
>> ETH_FCS_LEN.
>>
>> Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
>> Signed-off-by: Szymon Heidrich <[email protected]>
>> Reported-by: kernel test robot <[email protected]>
>
> the test robot did not report the fact that the packet length needed to
> be limited :(
>

Yes, I removed the Reported-by tag in V3 as suggested by Jakub.