[Intel-wired-lan] [PATCH 1/2] e100: fix length calculation in e100_get_regs_len

[Intel-wired-lan] [PATCH 1/2] e100: fix length calculation in e100_get_regs_len

[Jacob Keller]

commit abf9b902059f ("e100: cleanup unneeded math") tried to simplify
e100_get_regs_len and remove a double 'divide and then multiply'
calculation that the e100_reg_regs_len function did.

This change broke the size calculation entirely as it failed to account
for the fact that the numbered registers are actually 4 bytes wide and
not 1 byte. This resulted in a significant under allocation of the
register buffer used by e100_get_regs.

Fix this by properly multiplying the register count by u32 first before
adding the size of the dump buffer.

Fixes: abf9b902059f ("e100: cleanup unneeded math")
Reported-by: Felicitas Hetzelt <felicitashetzelt@gmail.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/e100.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
index 373eb027b925..588a59546d12 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -2441,7 +2441,11 @@ static void e100_get_drvinfo(struct net_device *netdev,
 static int e100_get_regs_len(struct net_device *netdev)
 {
 	struct nic *nic = netdev_priv(netdev);
-	return 1 + E100_PHY_REGS + sizeof(nic->mem->dump_buf);
+
+	/* We know the number of registers, and the size of the dump buffer.
+	 * Calculate the total size in bytes.
+	 */
+	return (1 + E100_PHY_REGS) * sizeof(u32) + sizeof(nic->mem->dump_buf);
 }
 
 static void e100_get_regs(struct net_device *netdev,
-- 
2.31.1.331.gb0c09ab8796f

[Intel-wired-lan] [PATCH 2/2] e100: fix buffer overrun in e100_get_regs

[Jacob Keller]

The e100_get_regs function is used to implement a simple register dump
for the e100 device. The data is broken into a couple of MAC control
registers, and then a series of PHY registers, followed by a memory dump
buffer.

The total length of the register dump is defined as (1 + E100_PHY_REGS)
* sizeof(u32) + sizeof(nic->mem->dump_buf).

The logic for filling in the PHY registers uses a convoluted inverted
count for loop which counts from E100_PHY_REGS (0x1C) down to 0, and
assigns the slots 1 + E100_PHY_REGS - i. The first loop iteration will
fill in [1] and the final loop iteration will fill in [1 + 0x1C]. This
is actually one more than the supposed number of PHY registers.

The memory dump buffer is then filled into the space at
[2 + E100_PHY_REGS] which will cause that memcpy to assign 4 bytes past
the total size.

The end result is that we overrun the total buffer size allocated by the
kernel, which could lead to a panic or other issues due to memory
corruption.

It is difficult to determine the actual total number of registers
here. The only 8255x datasheet I could find indicates there are 28 total
MDI registers. However, we're reading 29 here, and reading them in
reverse!

In addition, the ethtool e100 register dump interface appears to read
the first PHY register to determine if the device is in MDI or MDIx
mode. This doesn't appear to be documented anywhere within the 8255x
datasheet. I can only assume it must be in register 28 (the extra
register we're reading here).

Lets not change any of the intended meaning of what we copy here. Just
extend the space by 4 bytes to account for the extra register and
continue copying the data out in the same order.

Change the E100_PHY_REGS value to be the correct total (29) so that the
total register dump size is calculated properly. Fix the offset for
where we copy the dump buffer so that it doesn't overrun the total size.

Re-write the for loop to use counting up instead of the convoluted
down-counting. Correct the mdio_read offset to use the 0-based register
offsets, but maintain the bizarre reverse ordering so that we have the
ABI expected by applications like ethtool. This requires and additional
subtraction of 1. It seems a bit odd but it makes the flow of assignment
into the register buffer easier to follow.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Rerported-by: Reported-by: Felicitas Hetzelt <felicitashetzelt@gmail.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/e100.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
index 588a59546d12..7ac932e95fcb 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -2437,7 +2437,7 @@ static void e100_get_drvinfo(struct net_device *netdev,
 		sizeof(info->bus_info));
 }
 
-#define E100_PHY_REGS 0x1C
+#define E100_PHY_REGS 0x1D
 static int e100_get_regs_len(struct net_device *netdev)
 {
 	struct nic *nic = netdev_priv(netdev);
@@ -2459,13 +2459,17 @@ static void e100_get_regs(struct net_device *netdev,
 	buff[0] = ioread8(&nic->csr->scb.cmd_hi) << 24 |
 		ioread8(&nic->csr->scb.cmd_lo) << 16 |
 		ioread16(&nic->csr->scb.status);
-	for (i = E100_PHY_REGS; i >= 0; i--)
-		buff[1 + E100_PHY_REGS - i] =
-			mdio_read(netdev, nic->mii.phy_id, i);
+	for (i = 0; i < E100_PHY_REGS; i++)
+		/* Note that we read the registers in reverse order. This
+		 * ordering is the ABI apparently used by ethtool and other
+		 * applications.
+		 */
+		buff[1 + i] = mdio_read(netdev, nic->mii.phy_id,
+					E100_PHY_REGS - 1 - i);
 	memset(nic->mem->dump_buf, 0, sizeof(nic->mem->dump_buf));
 	e100_exec_cb(nic, NULL, e100_dump);
 	msleep(10);
-	memcpy(&buff[2 + E100_PHY_REGS], nic->mem->dump_buf,
+	memcpy(&buff[1 + E100_PHY_REGS], nic->mem->dump_buf,
 		sizeof(nic->mem->dump_buf));
 }
 
-- 

I spent rather longer than I intended on this. The fix is more involved than
I had originally intended. However, I think this is the only solution which
both avoids buffer overrun and prevents breaking the expected ABI between
ethtool and the driver.

2.31.1.331.gb0c09ab8796f

[Intel-wired-lan] [PATCH 2/2] e100: fix buffer overrun in e100_get_regs

[Keller, Jacob E]

On 9/8/2021 10:52 AM, Keller, Jacob E wrote:
> The e100_get_regs function is used to implement a simple register dump
> for the e100 device. The data is broken into a couple of MAC control
> registers, and then a series of PHY registers, followed by a memory dump
> buffer.
> 
> The total length of the register dump is defined as (1 + E100_PHY_REGS)
> * sizeof(u32) + sizeof(nic->mem->dump_buf).
> 
> The logic for filling in the PHY registers uses a convoluted inverted
> count for loop which counts from E100_PHY_REGS (0x1C) down to 0, and
> assigns the slots 1 + E100_PHY_REGS - i. The first loop iteration will
> fill in [1] and the final loop iteration will fill in [1 + 0x1C]. This
> is actually one more than the supposed number of PHY registers.
> 
> The memory dump buffer is then filled into the space at
> [2 + E100_PHY_REGS] which will cause that memcpy to assign 4 bytes past
> the total size.
> 
> The end result is that we overrun the total buffer size allocated by the
> kernel, which could lead to a panic or other issues due to memory
> corruption.
> 
> It is difficult to determine the actual total number of registers
> here. The only 8255x datasheet I could find indicates there are 28 total
> MDI registers. However, we're reading 29 here, and reading them in
> reverse!
> 
> In addition, the ethtool e100 register dump interface appears to read
> the first PHY register to determine if the device is in MDI or MDIx
> mode. This doesn't appear to be documented anywhere within the 8255x
> datasheet. I can only assume it must be in register 28 (the extra
> register we're reading here).
> 
> Lets not change any of the intended meaning of what we copy here. Just
> extend the space by 4 bytes to account for the extra register and
> continue copying the data out in the same order.
> 
> Change the E100_PHY_REGS value to be the correct total (29) so that the
> total register dump size is calculated properly. Fix the offset for
> where we copy the dump buffer so that it doesn't overrun the total size.
> 
> Re-write the for loop to use counting up instead of the convoluted
> down-counting. Correct the mdio_read offset to use the 0-based register
> offsets, but maintain the bizarre reverse ordering so that we have the
> ABI expected by applications like ethtool. This requires and additional
> subtraction of 1. It seems a bit odd but it makes the flow of assignment
> into the register buffer easier to follow.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Rerported-by: Reported-by: Felicitas Hetzelt <felicitashetzelt@gmail.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/e100.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
> index 588a59546d12..7ac932e95fcb 100644
> --- a/drivers/net/ethernet/intel/e100.c
> +++ b/drivers/net/ethernet/intel/e100.c
> @@ -2437,7 +2437,7 @@ static void e100_get_drvinfo(struct net_device *netdev,
>  		sizeof(info->bus_info));
>  }
>  
> -#define E100_PHY_REGS 0x1C
> +#define E100_PHY_REGS 0x1D
>  static int e100_get_regs_len(struct net_device *netdev)
>  {
>  	struct nic *nic = netdev_priv(netdev);
> @@ -2459,13 +2459,17 @@ static void e100_get_regs(struct net_device *netdev,
>  	buff[0] = ioread8(&nic->csr->scb.cmd_hi) << 24 |
>  		ioread8(&nic->csr->scb.cmd_lo) << 16 |
>  		ioread16(&nic->csr->scb.status);
> -	for (i = E100_PHY_REGS; i >= 0; i--)
> -		buff[1 + E100_PHY_REGS - i] =
> -			mdio_read(netdev, nic->mii.phy_id, i);
> +	for (i = 0; i < E100_PHY_REGS; i++)
> +		/* Note that we read the registers in reverse order. This
> +		 * ordering is the ABI apparently used by ethtool and other
> +		 * applications.
> +		 */
> +		buff[1 + i] = mdio_read(netdev, nic->mii.phy_id,
> +					E100_PHY_REGS - 1 - i);
>  	memset(nic->mem->dump_buf, 0, sizeof(nic->mem->dump_buf));
>  	e100_exec_cb(nic, NULL, e100_dump);
>  	msleep(10);
> -	memcpy(&buff[2 + E100_PHY_REGS], nic->mem->dump_buf,
> +	memcpy(&buff[1 + E100_PHY_REGS], nic->mem->dump_buf,
>  		sizeof(nic->mem->dump_buf));
>  }
> 
I don't think this is quite right yet.... I loaded this up in QEMU with
an i82550 emulated nic. I still see a KASAN error with this applied. I'm
verifying everything again and will get back once I've got a proper fix
that doesn't trigger KASAN.

Thanks,
Jake

[Intel-wired-lan] [PATCH 2/2] e100: fix buffer overrun in e100_get_regs

[Keller, Jacob E]

On 9/9/2021 6:59 PM, Keller, Jacob E wrote:
> On 9/8/2021 10:52 AM, Keller, Jacob E wrote:
>> The e100_get_regs function is used to implement a simple register dump
>> for the e100 device. The data is broken into a couple of MAC control
>> registers, and then a series of PHY registers, followed by a memory dump
>> buffer.
>>
>> The total length of the register dump is defined as (1 + E100_PHY_REGS)
>> * sizeof(u32) + sizeof(nic->mem->dump_buf).
>>
>> The logic for filling in the PHY registers uses a convoluted inverted
>> count for loop which counts from E100_PHY_REGS (0x1C) down to 0, and
>> assigns the slots 1 + E100_PHY_REGS - i. The first loop iteration will
>> fill in [1] and the final loop iteration will fill in [1 + 0x1C]. This
>> is actually one more than the supposed number of PHY registers.
>>
>> The memory dump buffer is then filled into the space at
>> [2 + E100_PHY_REGS] which will cause that memcpy to assign 4 bytes past
>> the total size.
>>
>> The end result is that we overrun the total buffer size allocated by the
>> kernel, which could lead to a panic or other issues due to memory
>> corruption.
>>
>> It is difficult to determine the actual total number of registers
>> here. The only 8255x datasheet I could find indicates there are 28 total
>> MDI registers. However, we're reading 29 here, and reading them in
>> reverse!
>>
>> In addition, the ethtool e100 register dump interface appears to read
>> the first PHY register to determine if the device is in MDI or MDIx
>> mode. This doesn't appear to be documented anywhere within the 8255x
>> datasheet. I can only assume it must be in register 28 (the extra
>> register we're reading here).
>>
>> Lets not change any of the intended meaning of what we copy here. Just
>> extend the space by 4 bytes to account for the extra register and
>> continue copying the data out in the same order.
>>
>> Change the E100_PHY_REGS value to be the correct total (29) so that the
>> total register dump size is calculated properly. Fix the offset for
>> where we copy the dump buffer so that it doesn't overrun the total size.
>>
>> Re-write the for loop to use counting up instead of the convoluted
>> down-counting. Correct the mdio_read offset to use the 0-based register
>> offsets, but maintain the bizarre reverse ordering so that we have the
>> ABI expected by applications like ethtool. This requires and additional
>> subtraction of 1. It seems a bit odd but it makes the flow of assignment
>> into the register buffer easier to follow.
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Rerported-by: Reported-by: Felicitas Hetzelt <felicitashetzelt@gmail.com>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>  drivers/net/ethernet/intel/e100.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
>> index 588a59546d12..7ac932e95fcb 100644
>> --- a/drivers/net/ethernet/intel/e100.c
>> +++ b/drivers/net/ethernet/intel/e100.c
>> @@ -2437,7 +2437,7 @@ static void e100_get_drvinfo(struct net_device *netdev,
>>  		sizeof(info->bus_info));
>>  }
>>  
>> -#define E100_PHY_REGS 0x1C
>> +#define E100_PHY_REGS 0x1D
>>  static int e100_get_regs_len(struct net_device *netdev)
>>  {
>>  	struct nic *nic = netdev_priv(netdev);
>> @@ -2459,13 +2459,17 @@ static void e100_get_regs(struct net_device *netdev,
>>  	buff[0] = ioread8(&nic->csr->scb.cmd_hi) << 24 |
>>  		ioread8(&nic->csr->scb.cmd_lo) << 16 |
>>  		ioread16(&nic->csr->scb.status);
>> -	for (i = E100_PHY_REGS; i >= 0; i--)
>> -		buff[1 + E100_PHY_REGS - i] =
>> -			mdio_read(netdev, nic->mii.phy_id, i);
>> +	for (i = 0; i < E100_PHY_REGS; i++)
>> +		/* Note that we read the registers in reverse order. This
>> +		 * ordering is the ABI apparently used by ethtool and other
>> +		 * applications.
>> +		 */
>> +		buff[1 + i] = mdio_read(netdev, nic->mii.phy_id,
>> +					E100_PHY_REGS - 1 - i);
>>  	memset(nic->mem->dump_buf, 0, sizeof(nic->mem->dump_buf));
>>  	e100_exec_cb(nic, NULL, e100_dump);
>>  	msleep(10);
>> -	memcpy(&buff[2 + E100_PHY_REGS], nic->mem->dump_buf,
>> +	memcpy(&buff[1 + E100_PHY_REGS], nic->mem->dump_buf,
>>  		sizeof(nic->mem->dump_buf));
>>  }
>>
> I don't think this is quite right yet.... I loaded this up in QEMU with
> an i82550 emulated nic. I still see a KASAN error with this applied. I'm
> verifying everything again and will get back once I've got a proper fix
> that doesn't trigger KASAN.
> 
> Thanks,
> Jake
> 

I re-ran a build and made absolutely sure I got the right driver
installed. Once I did that the KASAN warnings went away. I suspect that
the build I was testing with the above comment was somehow not complete
or wasn't saved right.

With that being said, I can confidently say this is correct at least as
far as a simulation from qemu-kvm with the i82550 is concerned.

A bit odd maybe, but...

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

[Intel-wired-lan] [PATCH 1/2] e100: fix length calculation in e100_get_regs_len

[Keller, Jacob E]

On 9/8/2021 10:52 AM, Keller, Jacob E wrote:
> commit abf9b902059f ("e100: cleanup unneeded math") tried to simplify
> e100_get_regs_len and remove a double 'divide and then multiply'
> calculation that the e100_reg_regs_len function did.
> 
> This change broke the size calculation entirely as it failed to account
> for the fact that the numbered registers are actually 4 bytes wide and
> not 1 byte. This resulted in a significant under allocation of the
> register buffer used by e100_get_regs.
> 
> Fix this by properly multiplying the register count by u32 first before
> adding the size of the dump buffer.
> 
> Fixes: abf9b902059f ("e100: cleanup unneeded math")
> Reported-by: Felicitas Hetzelt <felicitashetzelt@gmail.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/e100.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
> index 373eb027b925..588a59546d12 100644
> --- a/drivers/net/ethernet/intel/e100.c
> +++ b/drivers/net/ethernet/intel/e100.c
> @@ -2441,7 +2441,11 @@ static void e100_get_drvinfo(struct net_device *netdev,
>  static int e100_get_regs_len(struct net_device *netdev)
>  {
>  	struct nic *nic = netdev_priv(netdev);
> -	return 1 + E100_PHY_REGS + sizeof(nic->mem->dump_buf);
> +
> +	/* We know the number of registers, and the size of the dump buffer.
> +	 * Calculate the total size in bytes.
> +	 */
> +	return (1 + E100_PHY_REGS) * sizeof(u32) + sizeof(nic->mem->dump_buf);
>  }
>  
>  static void e100_get_regs(struct net_device *netdev,
> 

For what it's worth, without this applied, CONFIG_KASAN shows something
along these lines:

> [   65.615306] ==================================================================
> [   65.615564] BUG: KASAN: vmalloc-out-of-bounds in dev_ethtool+0x1c30/0x3280
> [   65.615806] Write of size 596 at addr ffffc900001f1078 by task ethtool/1044
> 
> [   65.616070] CPU: 9 PID: 1044 Comm: ethtool Not tainted 5.14.0 #1
> [   65.616246] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-4.fc34 04/01/2014
> [   65.616486] Call Trace:
> [   65.616581]  dump_stack_lvl+0x46/0x5a
> [   65.616752]  print_address_description.constprop.0+0x1f/0x140
> [   65.616959]  ? dev_ethtool+0x1c30/0x3280
> [   65.617079]  kasan_report.cold+0x7f/0x11b
> [   65.617228]  ? dev_ethtool+0x1c30/0x3280
> [   65.617349]  kasan_check_range+0xf5/0x1d0
> [   65.617478]  memcpy+0x39/0x60
> [   65.617580]  dev_ethtool+0x1c30/0x3280
> [   65.617716]  ? ethtool_get_module_info_call+0xf0/0xf0
> [   65.617868]  ? post_alloc_hook+0xd9/0x120
> [   65.617995]  ? stack_trace_save+0x81/0xa0
> [   65.618127]  ? inet_ioctl+0x132/0x2c0
> [   65.618259]  ? inet_compat_ioctl+0x80/0x80
> [   65.618385]  ? avc_ss_reset+0xb0/0xb0
> [   65.618514]  ? cgroup_rstat_updated+0x61/0x180
> [   65.618669]  ? __alloc_pages_slowpath.constprop.0+0x1210/0x1210
> [   65.618852]  ? mutex_lock+0x7e/0xb0
> [   65.618981]  ? __mutex_lock_slowpath+0x10/0x10
> [   65.619124]  dev_ioctl+0x1b0/0x5c0
> [   65.619242]  sock_do_ioctl+0x146/0x1f0
> [   65.619369]  ? sock_alloc_file+0xd0/0xd0
> [   65.619486]  ? __handle_mm_fault+0x1201/0x1c00
> [   65.619629]  ? vm_iomap_memory+0xe0/0xe0
> [   65.619763]  ? userfaultfd_unmap_complete+0x7d/0x1c0
> [   65.619927]  sock_ioctl+0x332/0x430
> [   65.620044]  ? vlan_ioctl_set+0x30/0x30
> [   65.620162]  ? rwsem_mark_wake+0x460/0x460
> [   65.620291]  ? handle_mm_fault+0x17f/0x370
> [   65.620427]  __x64_sys_ioctl+0xb9/0xf0
> [   65.620541]  do_syscall_64+0x3b/0x90
> [   65.620672]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> [   65.620823] RIP: 0033:0x7f1504b1a0ab
> [   65.620933] Code: ff ff ff 85 c0 79 9b 49 c7 c4 ff ff ff ff 5b 5d 4c 89 e0 41 5c c3 66 0f 1f 84 00 00 00 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 9d bd 0c 00 f7 d8 64 89 01 48
> [   65.621422] RSP: 002b:00007fff451d5428 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> [   65.621650] RAX: ffffffffffffffda RBX: 00007fff451d56d0 RCX: 00007f1504b1a0ab
> [   65.621843] RDX: 00007fff451d56e0 RSI: 0000000000008946 RDI: 0000000000000003
> [   65.626946] RBP: 0000000000000271 R08: 0000562b6f8442a0 R09: 00007f1504be6a60
> [   65.631874] R10: fffffffffffff000 R11: 0000000000000246 R12: 00007fff451d5580
> [   65.635136] R13: 0000562b6d9538a0 R14: 00007fff451d56e0 R15: 0000000000000000
> 
> 
> [   65.643571] Memory state around the buggy address:
> [   65.645605]  ffffc900001f1100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [   65.647666]  ffffc900001f1180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [   65.649692] >ffffc900001f1200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 f8
> [   65.651548]                                                              ^
> [   65.652882]  ffffc900001f1280: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
> [   65.654243]  ffffc900001f1300: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
> [   65.655586] ==================================================================
With just this applied, the problem isn't fully resolved. However, I
really do think these are "separate" fixes. If desired, I'm happy to
merge them together.