diff mbox series

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

Message ID 20230306191824.4115839-1-harshit.m.mogalapalli@oracle.com
State New
Headers
Series [next] ca8210: Fix unsigned mac_len comparison with zero in ca8210_skb_tx() |

Commit Message

Harshit Mogalapalli March 6, 2023, 7:18 p.m. UTC 
  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(-)

Comments

Alexander Aring March 7, 2023, 12:06 a.m. UTC | #1 
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
Simon Horman March 7, 2023, 8:42 a.m. UTC | #2 
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>
Alexander Aring March 7, 2023, 11:29 p.m. UTC | #3 
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
Stefan Schmidt March 16, 2023, 4:31 p.m. UTC | #4 
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
Stefan Schmidt March 16, 2023, 4:33 p.m. UTC | #5 
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
Harshit Mogalapalli March 16, 2023, 7:02 p.m. UTC | #6 
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
diff mbox series

Patch

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