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 <[email protected]>
---
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
Hi,
On Mon, Mar 6, 2023 at 2:20 PM Harshit Mogalapalli
<[email protected]> 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 <[email protected]>
Acked-by: Alexander Aring <[email protected]>
sorry, I didn't see that... Thanks for sending this patch.
- Alex
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 <[email protected]>
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 <[email protected]>
Hi,
On Tue, Mar 7, 2023 at 3:44 AM Simon Horman <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
>
sure, thanks for catching this.
- Alex
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>
> 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 <[email protected]>
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
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 <[email protected]>
>> ---
>> 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