| Message ID | 20230306191824.4115839-1-harshit.m.mogalapalli@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [next] ca8210: Fix unsigned mac_len comparison with zero in ca8210_skb_tx() | expand |
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
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>
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
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
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
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 --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__);
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(-)