diff mbox series

net/bonding: fix socket id check

Message ID 1619075569-33619-1-git-send-email-humin29@huawei.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series net/bonding: fix socket id check |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

humin (Q) April 22, 2021, 7:12 a.m. UTC 
  From: Chengchang Tang <tangchengchang@huawei.com>

The socket ID entered by user is cast to an unsigned integer. However,
the value may be an illegal negative value, which may cause some
problems. In this case, an error should be returned.

In addition, the socket ID may be an invalid positive number, which is
also processed in this patch.

Fixes: 2efb58cbab6e ("bond: new link bonding library")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/bonding/rte_eth_bond_args.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Ferruh Yigit April 26, 2021, 2:54 p.m. UTC | #1 
On 4/22/2021 8:12 AM, Min Hu (Connor) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> The socket ID entered by user is cast to an unsigned integer. However,
> the value may be an illegal negative value, which may cause some
> problems. In this case, an error should be returned.
> 

+1 to fix

> In addition, the socket ID may be an invalid positive number, which is
> also processed in this patch.
> 
> Fixes: 2efb58cbab6e ("bond: new link bonding library")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  drivers/net/bonding/rte_eth_bond_args.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
> index 8c5f90d..bcc0fe3 100644
> --- a/drivers/net/bonding/rte_eth_bond_args.c
> +++ b/drivers/net/bonding/rte_eth_bond_args.c
> @@ -207,12 +207,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
>  		return -1;
>  
>  	errno = 0;
> -	socket_id = (uint8_t)strtol(value, &endptr, 10);
> +	socket_id = strtol(value, &endptr, 10);

'strtol()' returns 'long int', but implicitly casting it to 'int'. My concern is
if this cause a static analysis tool warning.
What do you think to have 'socket_id' type as 'long int'?

>  	if (*endptr != 0 || errno != 0)
>  		return -1;
>  
>  	/* validate socket id value */
> -	if (socket_id >= 0) {
> +	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {>  		*(uint8_t *)extra_args = (uint8_t)socket_id;

Here there is an assumption that RTE_MAX_NUMA_NODES will be less than
'UCHAR_MAX', perhaps it can be good to add a check to verify this assumption.

>  		return 0;
>  	}
>
Chengchang Tang April 27, 2021, 2:44 a.m. UTC | #2 
On 2021/4/26 22:54, Ferruh Yigit wrote:
> On 4/22/2021 8:12 AM, Min Hu (Connor) wrote:
>> From: Chengchang Tang <tangchengchang@huawei.com>
>>
>> The socket ID entered by user is cast to an unsigned integer. However,
>> the value may be an illegal negative value, which may cause some
>> problems. In this case, an error should be returned.
>>
> 
> +1 to fix
> 
>> In addition, the socket ID may be an invalid positive number, which is
>> also processed in this patch.
>>
>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>  drivers/net/bonding/rte_eth_bond_args.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
>> index 8c5f90d..bcc0fe3 100644
>> --- a/drivers/net/bonding/rte_eth_bond_args.c
>> +++ b/drivers/net/bonding/rte_eth_bond_args.c
>> @@ -207,12 +207,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
>>  		return -1;
>>  
>>  	errno = 0;
>> -	socket_id = (uint8_t)strtol(value, &endptr, 10);
>> +	socket_id = strtol(value, &endptr, 10);
> 
> 'strtol()' returns 'long int', but implicitly casting it to 'int'. My concern is
> if this cause a static analysis tool warning.
> What do you think to have 'socket_id' type as 'long int'?
> 
I think it would be better to cast to the 'int' here, for reasons below.

>>  	if (*endptr != 0 || errno != 0)
>>  		return -1;
>>  
>>  	/* validate socket id value */
>> -	if (socket_id >= 0) {
>> +	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {>  		*(uint8_t *)extra_args = (uint8_t)socket_id;
> 
> Here there is an assumption that RTE_MAX_NUMA_NODES will be less than
> 'UCHAR_MAX', perhaps it can be good to add a check to verify this assumption.

Currently, it is unlikely that RTE_MAX_NUMA_NODES will be greater than 256. Therefore,
adding such check will not cause any problems. But I don't think it's necessary to put
such restrictions on it (i.e. RTE_MAX_NUMA_NODES should be less than UCHAR_MAX).
I checked all references to RTE_MAX_NUMA_NODES, and usually socket_id is of type 'int'
or 'unsigned int' (Only the efd, node, and bonding specify 'unsigned char' for socket
IDs.). And for that, I think it will be better to change the socket id type to 'int'
in this patch. For the type of socket id in efd and node, I will send new patches to
modify it.
> 
>>  		return 0;
>>  	}
>>
> 
> 
> .
>
Ferruh Yigit April 27, 2021, 10:45 a.m. UTC | #3 
On 4/27/2021 3:44 AM, Chengchang Tang wrote:
> 
> 
> On 2021/4/26 22:54, Ferruh Yigit wrote:
>> On 4/22/2021 8:12 AM, Min Hu (Connor) wrote:
>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>
>>> The socket ID entered by user is cast to an unsigned integer. However,
>>> the value may be an illegal negative value, which may cause some
>>> problems. In this case, an error should be returned.
>>>
>>
>> +1 to fix
>>
>>> In addition, the socket ID may be an invalid positive number, which is
>>> also processed in this patch.
>>>
>>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> ---
>>>  drivers/net/bonding/rte_eth_bond_args.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
>>> index 8c5f90d..bcc0fe3 100644
>>> --- a/drivers/net/bonding/rte_eth_bond_args.c
>>> +++ b/drivers/net/bonding/rte_eth_bond_args.c
>>> @@ -207,12 +207,12 @@ bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
>>>  		return -1;
>>>  
>>>  	errno = 0;
>>> -	socket_id = (uint8_t)strtol(value, &endptr, 10);
>>> +	socket_id = strtol(value, &endptr, 10);
>>
>> 'strtol()' returns 'long int', but implicitly casting it to 'int'. My concern is
>> if this cause a static analysis tool warning.
>> What do you think to have 'socket_id' type as 'long int'?
>>
> I think it would be better to cast to the 'int' here, for reasons below.
> 

Independent from below reasons, converting from user provided "long int" to
'int' will cause losing value and may lead wrong checks,

Like if user provided '-4294967281' (0xffffffff0000000f), when you cast to
'int', it will become '15' (0xf) and will pass from validation checks.

So I think better to verify the value first as "long int", later cast it to 'int'.

>>>  	if (*endptr != 0 || errno != 0)
>>>  		return -1;
>>>  
>>>  	/* validate socket id value */
>>> -	if (socket_id >= 0) {
>>> +	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {>  		*(uint8_t *)extra_args = (uint8_t)socket_id;
>>
>> Here there is an assumption that RTE_MAX_NUMA_NODES will be less than
>> 'UCHAR_MAX', perhaps it can be good to add a check to verify this assumption.
> 
> Currently, it is unlikely that RTE_MAX_NUMA_NODES will be greater than 256. Therefore,
> adding such check will not cause any problems. But I don't think it's necessary to put
> such restrictions on it (i.e. RTE_MAX_NUMA_NODES should be less than UCHAR_MAX).

Restriction comes from provided 'extra_args' being 'uint8_t', I just suggest
checking this.

> I checked all references to RTE_MAX_NUMA_NODES, and usually socket_id is of type 'int'
> or 'unsigned int' (Only the efd, node, and bonding specify 'unsigned char' for socket
> IDs.). And for that, I think it will be better to change the socket id type to 'int'
> in this patch. For the type of socket id in efd and node, I will send new patches to
> modify it.
>>
>>>  		return 0;
>>>  	}
>>>
>>
>>
>> .
>>
>
diff mbox series

Patch

diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index 8c5f90d..bcc0fe3 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -207,12 +207,12 @@  bond_ethdev_parse_socket_id_kvarg(const char *key __rte_unused,
 		return -1;
 
 	errno = 0;
-	socket_id = (uint8_t)strtol(value, &endptr, 10);
+	socket_id = strtol(value, &endptr, 10);
 	if (*endptr != 0 || errno != 0)
 		return -1;
 
 	/* validate socket id value */
-	if (socket_id >= 0) {
+	if (socket_id >= 0 && socket_id < RTE_MAX_NUMA_NODES) {
 		*(uint8_t *)extra_args = (uint8_t)socket_id;
 		return 0;
 	}