diff mbox series

net/txgbe: fix out of bound access

Message ID 20231116140718.4026676-1-ferruh.yigit@amd.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series net/txgbe: fix out of bound access |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/intel-Functional success Functional PASS

Commit Message

Ferruh Yigit Nov. 16, 2023, 2:07 p.m. UTC 
  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(+)

Comments

Luca Boccassi Nov. 16, 2023, 3:16 p.m. UTC | #1 
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>
Jiawen Wu Nov. 17, 2023, 2:45 a.m. UTC | #2 
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
>
Ferruh Yigit Nov. 17, 2023, 9:15 a.m. UTC | #3 
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
>>
>
diff mbox series

Patch

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);