[PATCH next] ca8210: Fix unsigned mac_len comparison with zero in ca8210_skb_tx()

[PATCH next] ca8210: Fix unsigned mac_len comparison with zero in ca8210_skb_tx()

[Harshit Mogalapalli]

mac_len is of type unsigned, which can never be less than zero.

	mac_len = ieee802154_hdr_peek_addrs(skb, &header);
	if (mac_len < 0)
		return mac_len;

Change this to type int as ieee802154_hdr_peek_addrs() can return negative
integers, this is found by static analysis with smatch.

Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
Only compile tested.
---
 drivers/net/ieee802154/ca8210.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
index 0b0c6c0764fe..d0b5129439ed 100644
--- a/drivers/net/ieee802154/ca8210.c
+++ b/drivers/net/ieee802154/ca8210.c
@@ -1902,10 +1902,9 @@ static int ca8210_skb_tx(
 	struct ca8210_priv  *priv
 )
 {
-	int status;
 	struct ieee802154_hdr header = { };
 	struct secspec secspec;
-	unsigned int mac_len;
+	int mac_len, status;
 
 	dev_dbg(&priv->spi->dev, "%s called\n", __func__);
 
-- 
2.38.1

Re: [PATCH next] ca8210: Fix unsigned mac_len comparison with zero in ca8210_skb_tx()

[Alexander Aring]

Hi,

On Mon, Mar 6, 2023 at 2:20 PM Harshit Mogalapalli
<harshit.m.mogalapalli@oracle.com> wrote:
>
> mac_len is of type unsigned, which can never be less than zero.
>
>         mac_len = ieee802154_hdr_peek_addrs(skb, &header);
>         if (mac_len < 0)
>                 return mac_len;
>
> Change this to type int as ieee802154_hdr_peek_addrs() can return negative
> integers, this is found by static analysis with smatch.
>
> Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>

Acked-by: Alexander Aring <aahringo@redhat.com>

sorry, I didn't see that... Thanks for sending this patch.

- Alex

Re: [PATCH next] ca8210: Fix unsigned mac_len comparison with zero in ca8210_skb_tx()

[Simon Horman]

On Mon, Mar 06, 2023 at 11:18:24AM -0800, Harshit Mogalapalli wrote:
> mac_len is of type unsigned, which can never be less than zero.
> 
> 	mac_len = ieee802154_hdr_peek_addrs(skb, &header);
> 	if (mac_len < 0)
> 		return mac_len;
> 
> Change this to type int as ieee802154_hdr_peek_addrs() can return negative
> integers, this is found by static analysis with smatch.
> 
> Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>

I discussed this briefly with Harshit offline.

The commit referenced above tag does add the call to
ieee802154_hdr_peek_addrs(), an there is a sign miss match between
the return value and the variable.

The code to check the mac_len was added more recently, by the following
commit. However the fixes tag is probably fine as-is, because it's fixing
error handling of a call made in that commit.

6c993779ea1d ("ca8210: fix mac_len negative array access")

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH next] ca8210: Fix unsigned mac_len comparison with zero in ca8210_skb_tx()

[Alexander Aring]

Hi,

On Tue, Mar 7, 2023 at 3:44 AM Simon Horman <simon.horman@corigine.com> wrote:
>
> On Mon, Mar 06, 2023 at 11:18:24AM -0800, Harshit Mogalapalli wrote:
> > mac_len is of type unsigned, which can never be less than zero.
> >
> >       mac_len = ieee802154_hdr_peek_addrs(skb, &header);
> >       if (mac_len < 0)
> >               return mac_len;
> >
> > Change this to type int as ieee802154_hdr_peek_addrs() can return negative
> > integers, this is found by static analysis with smatch.
> >
> > Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
> > Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
>
> I discussed this briefly with Harshit offline.
>
> The commit referenced above tag does add the call to
> ieee802154_hdr_peek_addrs(), an there is a sign miss match between
> the return value and the variable.
>
> The code to check the mac_len was added more recently, by the following
> commit. However the fixes tag is probably fine as-is, because it's fixing
> error handling of a call made in that commit.
>
> 6c993779ea1d ("ca8210: fix mac_len negative array access")
>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>

sure, thanks for catching this.

- Alex

Re: [PATCH next] ca8210: Fix unsigned mac_len comparison with zero in ca8210_skb_tx()

[Stefan Schmidt]

Hello Simon.

On 07.03.23 09:42, Simon Horman wrote:
> On Mon, Mar 06, 2023 at 11:18:24AM -0800, Harshit Mogalapalli wrote:
>> mac_len is of type unsigned, which can never be less than zero.
>>
>> 	mac_len = ieee802154_hdr_peek_addrs(skb, &header);
>> 	if (mac_len < 0)
>> 		return mac_len;
>>
>> Change this to type int as ieee802154_hdr_peek_addrs() can return negative
>> integers, this is found by static analysis with smatch.
>>
>> Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
>> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> 
> I discussed this briefly with Harshit offline.
> 
> The commit referenced above tag does add the call to
> ieee802154_hdr_peek_addrs(), an there is a sign miss match between
> the return value and the variable.
> 
> The code to check the mac_len was added more recently, by the following
> commit. However the fixes tag is probably fine as-is, because it's fixing
> error handling of a call made in that commit.
> 
> 6c993779ea1d ("ca8210: fix mac_len negative array access")
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>

I agree that the commit above is the better Fixes tag as it makes clear 
it only comes after this change. I amended the commit message 
accordingly when applying this to wpan.

regards
Stefan Schmidt

Re: [PATCH next] ca8210: Fix unsigned mac_len comparison with zero in ca8210_skb_tx()

[Stefan Schmidt]

Hello Harshit.

On 06.03.23 20:18, Harshit Mogalapalli wrote:
> mac_len is of type unsigned, which can never be less than zero.
> 
> 	mac_len = ieee802154_hdr_peek_addrs(skb, &header);
> 	if (mac_len < 0)
> 		return mac_len;
> 
> Change this to type int as ieee802154_hdr_peek_addrs() can return negative
> integers, this is found by static analysis with smatch.
> 
> Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> ---
> Only compile tested.
> ---
>   drivers/net/ieee802154/ca8210.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
> index 0b0c6c0764fe..d0b5129439ed 100644
> --- a/drivers/net/ieee802154/ca8210.c
> +++ b/drivers/net/ieee802154/ca8210.c
> @@ -1902,10 +1902,9 @@ static int ca8210_skb_tx(
>   	struct ca8210_priv  *priv
>   )
>   {
> -	int status;
>   	struct ieee802154_hdr header = { };
>   	struct secspec secspec;
> -	unsigned int mac_len;
> +	int mac_len, status;
>   
>   	dev_dbg(&priv->spi->dev, "%s called\n", __func__);
>   

This patch has been applied to the wpan tree and will be
part of the next pull request to net. Thanks!

I took the liberty and changed the fixes tag to the change that 
introduced the resaon for the mismatch recently. As suggested by Simon.

regards
Stefan Schmidt

Re: [PATCH next] ca8210: Fix unsigned mac_len comparison with zero in ca8210_skb_tx()

[Harshit Mogalapalli]

Hi Stefan,

On 16/03/23 10:01 pm, Stefan Schmidt wrote:
> Hello Harshit.
> 
> On 06.03.23 20:18, Harshit Mogalapalli wrote:
>> mac_len is of type unsigned, which can never be less than zero.
>>
>>     mac_len = ieee802154_hdr_peek_addrs(skb, &header);
>>     if (mac_len < 0)
>>         return mac_len;
>>
>> Change this to type int as ieee802154_hdr_peek_addrs() can return 
>> negative
>> integers, this is found by static analysis with smatch.
>>
>> Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device 
>> driver")
>> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
>> ---
>> Only compile tested.
>> ---
>>   drivers/net/ieee802154/ca8210.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ieee802154/ca8210.c 
>> b/drivers/net/ieee802154/ca8210.c
>> index 0b0c6c0764fe..d0b5129439ed 100644
>> --- a/drivers/net/ieee802154/ca8210.c
>> +++ b/drivers/net/ieee802154/ca8210.c
>> @@ -1902,10 +1902,9 @@ static int ca8210_skb_tx(
>>       struct ca8210_priv  *priv
>>   )
>>   {
>> -    int status;
>>       struct ieee802154_hdr header = { };
>>       struct secspec secspec;
>> -    unsigned int mac_len;
>> +    int mac_len, status;
>>       dev_dbg(&priv->spi->dev, "%s called\n", __func__);
> 
> This patch has been applied to the wpan tree and will be
> part of the next pull request to net. Thanks!
> 
> I took the liberty and changed the fixes tag to the change that 
> introduced the resaon for the mismatch recently. As suggested by Simon.
> 

Thanks for doing this, I wasn't very clear whether to change the Fixes 
tag and send a v2.

Regards,
Harshit

> regards
> Stefan Schmidt