* Re: [PATCH] net/txgbe: fix out of bound access
2023-11-16 14:07 [PATCH] net/txgbe: fix out of bound access Ferruh Yigit
@ 2023-11-16 15:16 ` Luca Boccassi
2023-11-17 2:45 ` Jiawen Wu
2023-11-17 10:12 ` [PATCH v2] " Ferruh Yigit
2 siblings, 0 replies; 7+ messages in thread
From: Luca Boccassi @ 2023-11-16 15:16 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, stable
On Thu, 2023-11-16 at 14:07 +0000, Ferruh Yigit wrote:
> Reported by SuSe CI [1] by GCC [2], possibly false positive. Error:
>
> In function 'txgbe_host_interface_command',
> inlined from 'txgbe_host_interface_command'
> at ../drivers/net/txgbe/base/txgbe_mng.c:104:1,
> inlined from 'txgbe_hic_reset'
> at ../drivers/net/txgbe/base/txgbe_mng.c:345:9:
> ../drivers/net/txgbe/base/txgbe_mng.c:145:36:
> error: array subscript 2 is outside array bounds ofr
> 'struct txgbe_hic_reset[1]' [-Werror=array-bounds=]
> 145 | buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
> ../drivers/net/txgbe/base/txgbe_mng.c: In function 'txgbe_hic_reset':
> ../drivers/net/txgbe/base/txgbe_mng.c:331:32:
> note: at offset 8 into object 'reset_cmd' of size 8
> 331 | struct txgbe_hic_reset reset_cmd;
> | ^~~~~~~~~
>
> Access to buffer done based on command code, the case complained by
> FW_RESET_CMD has short buffer but this code path only taken with command
> 0x30, so this shouldn't be a problem.
>
> Adding a size check before accessing to the buffer, as this is control
> plane code, additional check shouldn't hurt.
>
> [1]
> https://build.opensuse.org/public/build/home:bluca:dpdk/openSUSE_Factory_ARM/armv7l/dpdk-20.11/_log
>
> [2]
> gcc 13.2.1 "cc (SUSE Linux) 13.2.1 20230912
>
> Fixes: 35c90ecccfd4 ("net/txgbe: add EEPROM functions")
> Cc: stable@dpdk.org
>
> Reported-by: Luca Boccassi <luca.boccassi@microsoft.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>
> ---
> Cc: jiawenwu@trustnetic.com
> Cc: jianwang@trustnetic.com
>
> @Luca, I am not sure if this additional check will satisfy the compiler,
> can you please verify the patch?
>
> @Jiawen, there is a specific handling for command 0x30, from comment it
> looks like it is Read Flash command, but it looks like this command is
> not used by the driver, if this is correct can we remove the check
> completely? Removing can be simpler way to fix the compiler error.
> ---
> drivers/net/txgbe/base/txgbe_mng.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/txgbe/base/txgbe_mng.c b/drivers/net/txgbe/base/txgbe_mng.c
> index df7145094f84..9797b1b8b5da 100644
> --- a/drivers/net/txgbe/base/txgbe_mng.c
> +++ b/drivers/net/txgbe/base/txgbe_mng.c
> @@ -147,6 +147,10 @@ txgbe_host_interface_command(struct txgbe_hw *hw, u32 *buffer,
> * two byes instead of one byte
> */
> if (resp->cmd == 0x30) {
> + if (length < ((dword_len + 2) << 2)) {
> + err = TXGBE_ERR_HOST_INTERFACE_COMMAND;
> + goto rel_out;
> + }
> for (; bi < dword_len + 2; bi++)
> buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
>
Thanks, this fixes the build:
https://build.opensuse.org/package/live_build_log/home:bluca:dpdk/dpdk-20.11/openSUSE_Factory_ARM/armv7l
Tested-by: Luca Boccassi <bluca@debian.org>
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH] net/txgbe: fix out of bound access
2023-11-16 14:07 [PATCH] net/txgbe: fix out of bound access Ferruh Yigit
2023-11-16 15:16 ` Luca Boccassi
@ 2023-11-17 2:45 ` Jiawen Wu
2023-11-17 9:15 ` Ferruh Yigit
2023-11-17 10:12 ` [PATCH v2] " Ferruh Yigit
2 siblings, 1 reply; 7+ messages in thread
From: Jiawen Wu @ 2023-11-17 2:45 UTC (permalink / raw)
To: Ferruh.Yigit, 'Jian Wang', 'Ferruh Yigit'
Cc: dev, stable, 'Luca Boccassi'
On Thursday, November 16, 2023 10:07 PM, Ferruh.Yigit@amd.com wrote:
> Reported by SuSe CI [1] by GCC [2], possibly false positive. Error:
>
> In function 'txgbe_host_interface_command',
> inlined from 'txgbe_host_interface_command'
> at ../drivers/net/txgbe/base/txgbe_mng.c:104:1,
> inlined from 'txgbe_hic_reset'
> at ../drivers/net/txgbe/base/txgbe_mng.c:345:9:
> ../drivers/net/txgbe/base/txgbe_mng.c:145:36:
> error: array subscript 2 is outside array bounds ofr
> 'struct txgbe_hic_reset[1]' [-Werror=array-bounds=]
> 145 | buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
> ../drivers/net/txgbe/base/txgbe_mng.c: In function 'txgbe_hic_reset':
> ../drivers/net/txgbe/base/txgbe_mng.c:331:32:
> note: at offset 8 into object 'reset_cmd' of size 8
> 331 | struct txgbe_hic_reset reset_cmd;
> | ^~~~~~~~~
>
> Access to buffer done based on command code, the case complained by
> FW_RESET_CMD has short buffer but this code path only taken with command
> 0x30, so this shouldn't be a problem.
>
> Adding a size check before accessing to the buffer, as this is control
> plane code, additional check shouldn't hurt.
>
> [1]
> https://build.opensuse.org/public/build/home:bluca:dpdk/openSUSE_Factory_ARM/armv7l/dpdk-20.11/_log
>
> [2]
> gcc 13.2.1 "cc (SUSE Linux) 13.2.1 20230912
>
> Fixes: 35c90ecccfd4 ("net/txgbe: add EEPROM functions")
> Cc: stable@dpdk.org
>
> Reported-by: Luca Boccassi <luca.boccassi@microsoft.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>
> ---
> Cc: jiawenwu@trustnetic.com
> Cc: jianwang@trustnetic.com
>
> @Luca, I am not sure if this additional check will satisfy the compiler,
> can you please verify the patch?
>
> @Jiawen, there is a specific handling for command 0x30, from comment it
> looks like it is Read Flash command, but it looks like this command is
> not used by the driver, if this is correct can we remove the check
> completely? Removing can be simpler way to fix the compiler error.
Thanks Ferruh. This command has been removed because flash can be read
directly by the driver. The check can be simply removed.
> ---
> drivers/net/txgbe/base/txgbe_mng.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/txgbe/base/txgbe_mng.c b/drivers/net/txgbe/base/txgbe_mng.c
> index df7145094f84..9797b1b8b5da 100644
> --- a/drivers/net/txgbe/base/txgbe_mng.c
> +++ b/drivers/net/txgbe/base/txgbe_mng.c
> @@ -147,6 +147,10 @@ txgbe_host_interface_command(struct txgbe_hw *hw, u32 *buffer,
> * two byes instead of one byte
> */
> if (resp->cmd == 0x30) {
> + if (length < ((dword_len + 2) << 2)) {
> + err = TXGBE_ERR_HOST_INTERFACE_COMMAND;
> + goto rel_out;
> + }
> for (; bi < dword_len + 2; bi++)
> buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] net/txgbe: fix out of bound access
2023-11-17 2:45 ` Jiawen Wu
@ 2023-11-17 9:15 ` Ferruh Yigit
0 siblings, 0 replies; 7+ messages in thread
From: Ferruh Yigit @ 2023-11-17 9:15 UTC (permalink / raw)
To: Jiawen Wu, 'Jian Wang', 'Ferruh Yigit'
Cc: dev, stable, 'Luca Boccassi'
On 11/17/2023 2:45 AM, Jiawen Wu wrote:
> On Thursday, November 16, 2023 10:07 PM, Ferruh.Yigit@amd.com wrote:
>> Reported by SuSe CI [1] by GCC [2], possibly false positive. Error:
>>
>> In function 'txgbe_host_interface_command',
>> inlined from 'txgbe_host_interface_command'
>> at ../drivers/net/txgbe/base/txgbe_mng.c:104:1,
>> inlined from 'txgbe_hic_reset'
>> at ../drivers/net/txgbe/base/txgbe_mng.c:345:9:
>> ../drivers/net/txgbe/base/txgbe_mng.c:145:36:
>> error: array subscript 2 is outside array bounds ofr
>> 'struct txgbe_hic_reset[1]' [-Werror=array-bounds=]
>> 145 | buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
>> ../drivers/net/txgbe/base/txgbe_mng.c: In function 'txgbe_hic_reset':
>> ../drivers/net/txgbe/base/txgbe_mng.c:331:32:
>> note: at offset 8 into object 'reset_cmd' of size 8
>> 331 | struct txgbe_hic_reset reset_cmd;
>> | ^~~~~~~~~
>>
>> Access to buffer done based on command code, the case complained by
>> FW_RESET_CMD has short buffer but this code path only taken with command
>> 0x30, so this shouldn't be a problem.
>>
>> Adding a size check before accessing to the buffer, as this is control
>> plane code, additional check shouldn't hurt.
>>
>> [1]
>> https://build.opensuse.org/public/build/home:bluca:dpdk/openSUSE_Factory_ARM/armv7l/dpdk-20.11/_log
>>
>> [2]
>> gcc 13.2.1 "cc (SUSE Linux) 13.2.1 20230912
>>
>> Fixes: 35c90ecccfd4 ("net/txgbe: add EEPROM functions")
>> Cc: stable@dpdk.org
>>
>> Reported-by: Luca Boccassi <luca.boccassi@microsoft.com>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>
>> ---
>> Cc: jiawenwu@trustnetic.com
>> Cc: jianwang@trustnetic.com
>>
>> @Luca, I am not sure if this additional check will satisfy the compiler,
>> can you please verify the patch?
>>
>> @Jiawen, there is a specific handling for command 0x30, from comment it
>> looks like it is Read Flash command, but it looks like this command is
>> not used by the driver, if this is correct can we remove the check
>> completely? Removing can be simpler way to fix the compiler error.
>
> Thanks Ferruh. This command has been removed because flash can be read
> directly by the driver. The check can be simply removed.
>
OK, I will send a new version for it.
>> ---
>> drivers/net/txgbe/base/txgbe_mng.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/txgbe/base/txgbe_mng.c b/drivers/net/txgbe/base/txgbe_mng.c
>> index df7145094f84..9797b1b8b5da 100644
>> --- a/drivers/net/txgbe/base/txgbe_mng.c
>> +++ b/drivers/net/txgbe/base/txgbe_mng.c
>> @@ -147,6 +147,10 @@ txgbe_host_interface_command(struct txgbe_hw *hw, u32 *buffer,
>> * two byes instead of one byte
>> */
>> if (resp->cmd == 0x30) {
>> + if (length < ((dword_len + 2) << 2)) {
>> + err = TXGBE_ERR_HOST_INTERFACE_COMMAND;
>> + goto rel_out;
>> + }
>> for (; bi < dword_len + 2; bi++)
>> buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
>>
>> --
>> 2.34.1
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] net/txgbe: fix out of bound access
2023-11-16 14:07 [PATCH] net/txgbe: fix out of bound access Ferruh Yigit
2023-11-16 15:16 ` Luca Boccassi
2023-11-17 2:45 ` Jiawen Wu
@ 2023-11-17 10:12 ` Ferruh Yigit
2023-11-20 1:51 ` Jiawen Wu
2 siblings, 1 reply; 7+ messages in thread
From: Ferruh Yigit @ 2023-11-17 10:12 UTC (permalink / raw)
To: Jiawen Wu, Jian Wang, Ferruh Yigit; +Cc: dev, stable, Luca Boccassi
Reported by SuSe CI [1] by GCC [2], possibly false positive. Error:
In function 'txgbe_host_interface_command',
inlined from 'txgbe_host_interface_command'
at ../drivers/net/txgbe/base/txgbe_mng.c:104:1,
inlined from 'txgbe_hic_reset'
at ../drivers/net/txgbe/base/txgbe_mng.c:345:9:
../drivers/net/txgbe/base/txgbe_mng.c:145:36:
error: array subscript 2 is outside array bounds ofr
'struct txgbe_hic_reset[1]' [-Werror=array-bounds=]
145 | buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
../drivers/net/txgbe/base/txgbe_mng.c: In function 'txgbe_hic_reset':
../drivers/net/txgbe/base/txgbe_mng.c:331:32:
note: at offset 8 into object 'reset_cmd' of size 8
331 | struct txgbe_hic_reset reset_cmd;
| ^~~~~~~~~
Access to buffer done based on command code, the case complained by
FW_RESET_CMD has short buffer but this code path only taken with command
0x30, so this shouldn't be a problem.
Command 0x30 no more used, removing this exception check that cause
build error.
[1]
https://build.opensuse.org/public/build/home:bluca:dpdk/openSUSE_Factory_ARM/armv7l/dpdk-20.11/_log
[2]
gcc 13.2.1 "cc (SUSE Linux) 13.2.1 20230912
Fixes: 35c90ecccfd4 ("net/txgbe: add EEPROM functions")
Cc: stable@dpdk.org
Reported-by: Luca Boccassi <luca.boccassi@microsoft.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>
---
Cc: jiawenwu@trustnetic.com
Cc: jianwang@trustnetic.com
v2:
* Removed exception check for command 0x30
---
drivers/net/txgbe/base/txgbe_mng.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/drivers/net/txgbe/base/txgbe_mng.c b/drivers/net/txgbe/base/txgbe_mng.c
index df7145094f84..029a0a1fe143 100644
--- a/drivers/net/txgbe/base/txgbe_mng.c
+++ b/drivers/net/txgbe/base/txgbe_mng.c
@@ -141,21 +141,7 @@ txgbe_host_interface_command(struct txgbe_hw *hw, u32 *buffer,
for (bi = 0; bi < dword_len; bi++)
buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
- /*
- * If there is any thing in data position pull it in
- * Read Flash command requires reading buffer length from
- * two byes instead of one byte
- */
- if (resp->cmd == 0x30) {
- for (; bi < dword_len + 2; bi++)
- buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
-
- buf_len = (((u16)(resp->cmd_or_resp.ret_status) << 3)
- & 0xF00) | resp->buf_len;
- hdr_size += (2 << 2);
- } else {
- buf_len = resp->buf_len;
- }
+ buf_len = resp->buf_len;
if (!buf_len)
goto rel_out;
--
2.34.1
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH v2] net/txgbe: fix out of bound access
2023-11-17 10:12 ` [PATCH v2] " Ferruh Yigit
@ 2023-11-20 1:51 ` Jiawen Wu
2023-11-20 9:53 ` Ferruh Yigit
0 siblings, 1 reply; 7+ messages in thread
From: Jiawen Wu @ 2023-11-20 1:51 UTC (permalink / raw)
To: Ferruh.Yigit, 'Jian Wang', 'Ferruh Yigit'
Cc: dev, stable, 'Luca Boccassi'
On Friday, November 17, 2023 6:12 PM, Ferruh.Yigit@amd.com wrote:
> Reported by SuSe CI [1] by GCC [2], possibly false positive. Error:
>
> In function 'txgbe_host_interface_command',
> inlined from 'txgbe_host_interface_command'
> at ../drivers/net/txgbe/base/txgbe_mng.c:104:1,
> inlined from 'txgbe_hic_reset'
> at ../drivers/net/txgbe/base/txgbe_mng.c:345:9:
> ../drivers/net/txgbe/base/txgbe_mng.c:145:36:
> error: array subscript 2 is outside array bounds ofr
> 'struct txgbe_hic_reset[1]' [-Werror=array-bounds=]
> 145 | buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
> ../drivers/net/txgbe/base/txgbe_mng.c: In function 'txgbe_hic_reset':
> ../drivers/net/txgbe/base/txgbe_mng.c:331:32:
> note: at offset 8 into object 'reset_cmd' of size 8
> 331 | struct txgbe_hic_reset reset_cmd;
> | ^~~~~~~~~
>
> Access to buffer done based on command code, the case complained by
> FW_RESET_CMD has short buffer but this code path only taken with command
> 0x30, so this shouldn't be a problem.
>
> Command 0x30 no more used, removing this exception check that cause
> build error.
>
> [1]
> https://build.opensuse.org/public/build/home:bluca:dpdk/openSUSE_Factory_ARM/armv7l/dpdk-20.11/_log
>
> [2]
> gcc 13.2.1 "cc (SUSE Linux) 13.2.1 20230912
>
> Fixes: 35c90ecccfd4 ("net/txgbe: add EEPROM functions")
> Cc: stable@dpdk.org
>
> Reported-by: Luca Boccassi <luca.boccassi@microsoft.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>
> ---
> Cc: jiawenwu@trustnetic.com
> Cc: jianwang@trustnetic.com
>
> v2:
> * Removed exception check for command 0x30
> ---
> drivers/net/txgbe/base/txgbe_mng.c | 16 +---------------
> 1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/drivers/net/txgbe/base/txgbe_mng.c b/drivers/net/txgbe/base/txgbe_mng.c
> index df7145094f84..029a0a1fe143 100644
> --- a/drivers/net/txgbe/base/txgbe_mng.c
> +++ b/drivers/net/txgbe/base/txgbe_mng.c
> @@ -141,21 +141,7 @@ txgbe_host_interface_command(struct txgbe_hw *hw, u32 *buffer,
> for (bi = 0; bi < dword_len; bi++)
> buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
>
> - /*
> - * If there is any thing in data position pull it in
> - * Read Flash command requires reading buffer length from
> - * two byes instead of one byte
> - */
> - if (resp->cmd == 0x30) {
> - for (; bi < dword_len + 2; bi++)
> - buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
> -
> - buf_len = (((u16)(resp->cmd_or_resp.ret_status) << 3)
> - & 0xF00) | resp->buf_len;
> - hdr_size += (2 << 2);
> - } else {
> - buf_len = resp->buf_len;
> - }
> + buf_len = resp->buf_len;
> if (!buf_len)
> goto rel_out;
>
> --
> 2.34.1
Thanks Ferruh,
Reviewed-by: Jiawen Wu <jiawenwu@trustnetic.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] net/txgbe: fix out of bound access
2023-11-20 1:51 ` Jiawen Wu
@ 2023-11-20 9:53 ` Ferruh Yigit
0 siblings, 0 replies; 7+ messages in thread
From: Ferruh Yigit @ 2023-11-20 9:53 UTC (permalink / raw)
To: Jiawen Wu, 'Jian Wang', 'Ferruh Yigit'
Cc: dev, stable, 'Luca Boccassi'
On 11/20/2023 1:51 AM, Jiawen Wu wrote:
> On Friday, November 17, 2023 6:12 PM, Ferruh.Yigit@amd.com wrote:
>> Reported by SuSe CI [1] by GCC [2], possibly false positive. Error:
>>
>> In function 'txgbe_host_interface_command',
>> inlined from 'txgbe_host_interface_command'
>> at ../drivers/net/txgbe/base/txgbe_mng.c:104:1,
>> inlined from 'txgbe_hic_reset'
>> at ../drivers/net/txgbe/base/txgbe_mng.c:345:9:
>> ../drivers/net/txgbe/base/txgbe_mng.c:145:36:
>> error: array subscript 2 is outside array bounds ofr
>> 'struct txgbe_hic_reset[1]' [-Werror=array-bounds=]
>> 145 | buffer[bi] = rd32a(hw, TXGBE_MNGMBX, bi);
>> ../drivers/net/txgbe/base/txgbe_mng.c: In function 'txgbe_hic_reset':
>> ../drivers/net/txgbe/base/txgbe_mng.c:331:32:
>> note: at offset 8 into object 'reset_cmd' of size 8
>> 331 | struct txgbe_hic_reset reset_cmd;
>> | ^~~~~~~~~
>>
>> Access to buffer done based on command code, the case complained by
>> FW_RESET_CMD has short buffer but this code path only taken with command
>> 0x30, so this shouldn't be a problem.
>>
>> Command 0x30 no more used, removing this exception check that cause
>> build error.
>>
>> [1]
>> https://build.opensuse.org/public/build/home:bluca:dpdk/openSUSE_Factory_ARM/armv7l/dpdk-20.11/_log
>>
>> [2]
>> gcc 13.2.1 "cc (SUSE Linux) 13.2.1 20230912
>>
>> Fixes: 35c90ecccfd4 ("net/txgbe: add EEPROM functions")
>> Cc: stable@dpdk.org
>>
>> Reported-by: Luca Boccassi <luca.boccassi@microsoft.com>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>>
> Reviewed-by: Jiawen Wu <jiawenwu@trustnetic.com>
>
Applied to dpdk-next-net/main, thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread