[bug report] mt76: implement functions to get the response skb for MCU calls

[bug report] mt76: implement functions to get the response skb for MCU calls

[Dan Carpenter]

Hello Felix Fietkau,

The patch ae5ad6272d25: "mt76: implement functions to get the
response skb for MCU calls" from Sep 30, 2020, leads to the following
Smatch static checker warning:

	drivers/net/wireless/mediatek/mt76/mt7921/mcu.c:1151 mt7921_mcu_get_eeprom()
	error: potentially dereferencing uninitialized 'skb'.

drivers/net/wireless/mediatek/mt76/mt7921/mcu.c
    1136 int mt7921_mcu_get_eeprom(struct mt7921_dev *dev, u32 offset)
    1137 {
    1138         struct mt7921_mcu_eeprom_info req = {
    1139                 .addr = cpu_to_le32(round_down(offset, 16)),
    1140         };
    1141         struct mt7921_mcu_eeprom_info *res;
    1142         struct sk_buff *skb;
    1143         int ret;
    1144         u8 *buf;
    1145 
    1146         ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD_EFUSE_ACCESS, &req,
    1147                                         sizeof(req), true, &skb);

If mt76_mcu_send_and_get_msg() calls the dev->mcu_ops->mcu_send_msg()
then "skb" is not initialized.

    1148         if (ret)
    1149                 return ret;
    1150 
--> 1151         res = (struct mt7921_mcu_eeprom_info *)skb->data;
    1152         buf = dev->mt76.eeprom.data + le32_to_cpu(res->addr);
    1153         memcpy(buf, res->data, 16);
    1154         dev_kfree_skb(skb);
    1155 
    1156         return 0;
    1157 }

regards,
dan carpenter

Re: [bug report] mt76: implement functions to get the response skb for MCU calls

[Johannes Berg]

On Fri, 2021-10-08 at 16:00 +0300, Dan Carpenter wrote:
> 
>     1146         ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD_EFUSE_ACCESS, &req,
>     1147                                         sizeof(req), true, &skb);
> 
> If mt76_mcu_send_and_get_msg() calls the dev->mcu_ops->mcu_send_msg()
> then "skb" is not initialized.
> 
>     1148         if (ret)
>     1149                 return ret;
>     1150 
> --> 1151         res = (struct mt7921_mcu_eeprom_info *)skb->data;

Looks like possibly 'skb' is always initialized if
mt76_mcu_send_and_get_msg() returns 0 (success)?

But I guess it'd be nicer to write that with ERR_PTR() and actually
*return* the pointer (or ERR_PTR), rather than the output parameter.

johannes

Re: [bug report] mt76: implement functions to get the response skb for MCU calls

[Dan Carpenter]

On Fri, Oct 08, 2021 at 04:03:10PM +0200, Johannes Berg wrote:
> On Fri, 2021-10-08 at 16:00 +0300, Dan Carpenter wrote:
> > 
> >     1146         ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD_EFUSE_ACCESS, &req,
> >     1147                                         sizeof(req), true, &skb);
> > 
> > If mt76_mcu_send_and_get_msg() calls the dev->mcu_ops->mcu_send_msg()
> > then "skb" is not initialized.
> > 
> >     1148         if (ret)
> >     1149                 return ret;
> >     1150 
> > --> 1151         res = (struct mt7921_mcu_eeprom_info *)skb->data;
> 
> Looks like possibly 'skb' is always initialized if
> mt76_mcu_send_and_get_msg() returns 0 (success)?
> 

This build is with cross function analysis enabled so Smatch looks for
that.

The problem is that the caller has to know if dev->mcu_ops->mcu_send_msg
is NULL or not because if it's non-NULL "skb" is not set.  Perhaps that
means it should be separated into two functions and we pick which one
to call depending on whether the pointer is set.

drivers/net/wireless/mediatek/mt76/mcu.c
    54  int mt76_mcu_send_and_get_msg(struct mt76_dev *dev, int cmd, const void *data,
    55                                int len, bool wait_resp, struct sk_buff **ret_skb)
                                                                                ^^^^^^^
This is the parameter.

    56  {
    57          struct sk_buff *skb;
    58  
    59          if (dev->mcu_ops->mcu_send_msg)
    60                  return dev->mcu_ops->mcu_send_msg(dev, cmd, data, len, wait_resp);

The function pointer doesn't set *ret_skb at all.

    61  
    62          skb = mt76_mcu_msg_alloc(dev, data, len);
    63          if (!skb)
    64                  return -ENOMEM;
    65  
    66          return mt76_mcu_skb_send_and_get_msg(dev, skb, cmd, wait_resp, ret_skb);

But this does.

    67  }

regards,
dan carpenter

Re: [bug report] mt76: implement functions to get the response skb for MCU calls

[Dan Carpenter]

On Fri, Oct 08, 2021 at 05:27:35PM +0300, Dan Carpenter wrote:
> On Fri, Oct 08, 2021 at 04:03:10PM +0200, Johannes Berg wrote:
> > On Fri, 2021-10-08 at 16:00 +0300, Dan Carpenter wrote:
> > > 
> > >     1146         ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD_EFUSE_ACCESS, &req,
> > >     1147                                         sizeof(req), true, &skb);
> > > 
> > > If mt76_mcu_send_and_get_msg() calls the dev->mcu_ops->mcu_send_msg()
> > > then "skb" is not initialized.
> > > 
> > >     1148         if (ret)
> > >     1149                 return ret;
> > >     1150 
> > > --> 1151         res = (struct mt7921_mcu_eeprom_info *)skb->data;
> > 
> > Looks like possibly 'skb' is always initialized if
> > mt76_mcu_send_and_get_msg() returns 0 (success)?
> > 
> 
> This build is with cross function analysis enabled so Smatch looks for
> that.

Btw, it turns out I basically completely disabled the Smatch check for
uninitialized variables a while back.

I've fixed it now so it's warning again, but I'm going through and
manually fixing stuff and adding hack arounds to silence false
positives.  So hopefully, I'll be able to enable it in the published
code soonish.

regards,
dan carpenter

Re: [bug report] mt76: implement functions to get the response skb for MCU calls

[Dan Carpenter]

I would like to revisit this question.  Last time I complained about
this Johannes responded but he misread what mt76_mcu_send_and_get_msg()
does.  I have looked at it as well and I also cannot explain what is
going on in that function.

I have looked at the callers and my first instinct is that maybe this
is dead stub code?  But then when I look at mt76x02u_mcu_send_msg() I
think "No, this is not stub code.  This should be returning the newly
allocated skb to the caller."

But then I think, surely at some point someone tested this code???  It
must be stub code.

Could we get some clarity on this?

regards,
dan carpenter

On Fri, Oct 08, 2021 at 05:27:35PM +0300, Dan Carpenter wrote:
> On Fri, Oct 08, 2021 at 04:03:10PM +0200, Johannes Berg wrote:
> > On Fri, 2021-10-08 at 16:00 +0300, Dan Carpenter wrote:
> > > 
> > >     1146         ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD_EFUSE_ACCESS, &req,
> > >     1147                                         sizeof(req), true, &skb);
> > > 
> > > If mt76_mcu_send_and_get_msg() calls the dev->mcu_ops->mcu_send_msg()
> > > then "skb" is not initialized.
> > > 
> > >     1148         if (ret)
> > >     1149                 return ret;
> > >     1150 
> > > --> 1151         res = (struct mt7921_mcu_eeprom_info *)skb->data;
> > 
> > Looks like possibly 'skb' is always initialized if
> > mt76_mcu_send_and_get_msg() returns 0 (success)?
> > 
> 
> This build is with cross function analysis enabled so Smatch looks for
> that.
> 
> The problem is that the caller has to know if dev->mcu_ops->mcu_send_msg
> is NULL or not because if it's non-NULL "skb" is not set.  Perhaps that
> means it should be separated into two functions and we pick which one
> to call depending on whether the pointer is set.
> 
> drivers/net/wireless/mediatek/mt76/mcu.c
>     54  int mt76_mcu_send_and_get_msg(struct mt76_dev *dev, int cmd, const void *data,
>     55                                int len, bool wait_resp, struct sk_buff **ret_skb)
>                                                                                 ^^^^^^^
> This is the parameter.
> 
>     56  {
>     57          struct sk_buff *skb;
>     58  
>     59          if (dev->mcu_ops->mcu_send_msg)
>     60                  return dev->mcu_ops->mcu_send_msg(dev, cmd, data, len, wait_resp);
> 
> The function pointer doesn't set *ret_skb at all.
> 
>     61  
>     62          skb = mt76_mcu_msg_alloc(dev, data, len);
>     63          if (!skb)
>     64                  return -ENOMEM;
>     65  
>     66          return mt76_mcu_skb_send_and_get_msg(dev, skb, cmd, wait_resp, ret_skb);
> 
> But this does.
> 
>     67  }
> 
> regards,
> dan carpenter

Re: [bug report] mt76: implement functions to get the response skb for MCU calls

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3240 bytes --]

> I would like to revisit this question.  Last time I complained about
> this Johannes responded but he misread what mt76_mcu_send_and_get_msg()
> does.  I have looked at it as well and I also cannot explain what is
> going on in that function.
> 
> I have looked at the callers and my first instinct is that maybe this
> is dead stub code?  But then when I look at mt76x02u_mcu_send_msg() I
> think "No, this is not stub code.  This should be returning the newly
> allocated skb to the caller."
> 
> But then I think, surely at some point someone tested this code???  It
> must be stub code.
> 
> Could we get some clarity on this?

for mt76x2 and mt76x0 we do not care of ret_skb (in fact we do not run
mt76_mcu_send_and_get_msg() directly but we rely on mt76_mcu_send_msg()).
For mt7921 we set mcu_skb_send_msg function pointer and not mcu_send_msg.
Moreover mt7921_mcu_get_eeprom() has been remove a while back.
Am I missing something?

Regards,
Lorenzo

> 
> regards,
> dan carpenter
> 
> On Fri, Oct 08, 2021 at 05:27:35PM +0300, Dan Carpenter wrote:
> > On Fri, Oct 08, 2021 at 04:03:10PM +0200, Johannes Berg wrote:
> > > On Fri, 2021-10-08 at 16:00 +0300, Dan Carpenter wrote:
> > > > 
> > > >     1146         ret = mt76_mcu_send_and_get_msg(&dev->mt76, MCU_EXT_CMD_EFUSE_ACCESS, &req,
> > > >     1147                                         sizeof(req), true, &skb);
> > > > 
> > > > If mt76_mcu_send_and_get_msg() calls the dev->mcu_ops->mcu_send_msg()
> > > > then "skb" is not initialized.
> > > > 
> > > >     1148         if (ret)
> > > >     1149                 return ret;
> > > >     1150 
> > > > --> 1151         res = (struct mt7921_mcu_eeprom_info *)skb->data;
> > > 
> > > Looks like possibly 'skb' is always initialized if
> > > mt76_mcu_send_and_get_msg() returns 0 (success)?
> > > 
> > 
> > This build is with cross function analysis enabled so Smatch looks for
> > that.
> > 
> > The problem is that the caller has to know if dev->mcu_ops->mcu_send_msg
> > is NULL or not because if it's non-NULL "skb" is not set.  Perhaps that
> > means it should be separated into two functions and we pick which one
> > to call depending on whether the pointer is set.
> > 
> > drivers/net/wireless/mediatek/mt76/mcu.c
> >     54  int mt76_mcu_send_and_get_msg(struct mt76_dev *dev, int cmd, const void *data,
> >     55                                int len, bool wait_resp, struct sk_buff **ret_skb)
> >                                                                                 ^^^^^^^
> > This is the parameter.
> > 
> >     56  {
> >     57          struct sk_buff *skb;
> >     58  
> >     59          if (dev->mcu_ops->mcu_send_msg)
> >     60                  return dev->mcu_ops->mcu_send_msg(dev, cmd, data, len, wait_resp);
> > 
> > The function pointer doesn't set *ret_skb at all.
> > 
> >     61  
> >     62          skb = mt76_mcu_msg_alloc(dev, data, len);
> >     63          if (!skb)
> >     64                  return -ENOMEM;
> >     65  
> >     66          return mt76_mcu_skb_send_and_get_msg(dev, skb, cmd, wait_resp, ret_skb);
> > 
> > But this does.
> > 
> >     67  }
> > 
> > regards,
> > dan carpenter

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [bug report] mt76: implement functions to get the response skb for MCU calls

[Dan Carpenter]

On Thu, Oct 13, 2022 at 06:25:54PM +0200, Lorenzo Bianconi wrote:
> > I would like to revisit this question.  Last time I complained about
> > this Johannes responded but he misread what mt76_mcu_send_and_get_msg()
> > does.  I have looked at it as well and I also cannot explain what is
> > going on in that function.
> > 
> > I have looked at the callers and my first instinct is that maybe this
> > is dead stub code?  But then when I look at mt76x02u_mcu_send_msg() I
> > think "No, this is not stub code.  This should be returning the newly
> > allocated skb to the caller."
> > 
> > But then I think, surely at some point someone tested this code???  It
> > must be stub code.
> > 
> > Could we get some clarity on this?
> 
> for mt76x2 and mt76x0 we do not care of ret_skb (in fact we do not run
> mt76_mcu_send_and_get_msg() directly but we rely on mt76_mcu_send_msg()).
> For mt7921 we set mcu_skb_send_msg function pointer and not mcu_send_msg.

Ah thanks...  It's easy enough to silence the warning in Smatch but I
was never sure if it wasn't a bug.

> Moreover mt7921_mcu_get_eeprom() has been remove a while back.
> Am I missing something?

There are 12 callers for mt76_mcu_send_and_get_msg() and 11 of them
assume that the "ret_skb" is initialized (i.e. they assume that
the ->mcu_send_msg op is not used) so I get 11 Smatch warnings from
this...

Why not just do something like below?  It moves the ->mcu_send_msg()
call to the only place where it won't cause a crash.

regards,
dan carpenter

diff --git a/drivers/net/wireless/mediatek/mt76/mcu.c b/drivers/net/wireless/mediatek/mt76/mcu.c
index a8cafa39a56d..6bf0b7d8daee 100644
--- a/drivers/net/wireless/mediatek/mt76/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mcu.c
@@ -58,9 +58,6 @@ int mt76_mcu_send_and_get_msg(struct mt76_dev *dev, int cmd, const void *data,
 {
 	struct sk_buff *skb;
 
-	if (dev->mcu_ops->mcu_send_msg)
-		return dev->mcu_ops->mcu_send_msg(dev, cmd, data, len, wait_resp);
-
 	skb = mt76_mcu_msg_alloc(dev, data, len);
 	if (!skb)
 		return -ENOMEM;
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 87db9498dea4..99f931c08da9 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -1383,6 +1383,9 @@ static inline int
 mt76_mcu_send_msg(struct mt76_dev *dev, int cmd, const void *data, int len,
 		  bool wait_resp)
 {
+	if (dev->mcu_ops->mcu_send_msg)
+		return dev->mcu_ops->mcu_send_msg(dev, cmd, data, len, wait_resp);
+
 	return mt76_mcu_send_and_get_msg(dev, cmd, data, len, wait_resp, NULL);
 }
 

Re: [bug report] mt76: implement functions to get the response skb for MCU calls

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2944 bytes --]

> On Thu, Oct 13, 2022 at 06:25:54PM +0200, Lorenzo Bianconi wrote:
> > > I would like to revisit this question.  Last time I complained about
> > > this Johannes responded but he misread what mt76_mcu_send_and_get_msg()
> > > does.  I have looked at it as well and I also cannot explain what is
> > > going on in that function.
> > > 
> > > I have looked at the callers and my first instinct is that maybe this
> > > is dead stub code?  But then when I look at mt76x02u_mcu_send_msg() I
> > > think "No, this is not stub code.  This should be returning the newly
> > > allocated skb to the caller."
> > > 
> > > But then I think, surely at some point someone tested this code???  It
> > > must be stub code.
> > > 
> > > Could we get some clarity on this?
> > 
> > for mt76x2 and mt76x0 we do not care of ret_skb (in fact we do not run
> > mt76_mcu_send_and_get_msg() directly but we rely on mt76_mcu_send_msg()).
> > For mt7921 we set mcu_skb_send_msg function pointer and not mcu_send_msg.
> 
> Ah thanks...  It's easy enough to silence the warning in Smatch but I
> was never sure if it wasn't a bug.
> 
> > Moreover mt7921_mcu_get_eeprom() has been remove a while back.
> > Am I missing something?
> 
> There are 12 callers for mt76_mcu_send_and_get_msg() and 11 of them
> assume that the "ret_skb" is initialized (i.e. they assume that
> the ->mcu_send_msg op is not used) so I get 11 Smatch warnings from
> this...
> 
> Why not just do something like below?  It moves the ->mcu_send_msg()
> call to the only place where it won't cause a crash.
> 
> regards,
> dan carpenter
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mcu.c b/drivers/net/wireless/mediatek/mt76/mcu.c
> index a8cafa39a56d..6bf0b7d8daee 100644
> --- a/drivers/net/wireless/mediatek/mt76/mcu.c
> +++ b/drivers/net/wireless/mediatek/mt76/mcu.c
> @@ -58,9 +58,6 @@ int mt76_mcu_send_and_get_msg(struct mt76_dev *dev, int cmd, const void *data,
>  {
>  	struct sk_buff *skb;
>  
> -	if (dev->mcu_ops->mcu_send_msg)
> -		return dev->mcu_ops->mcu_send_msg(dev, cmd, data, len, wait_resp);
> -
>  	skb = mt76_mcu_msg_alloc(dev, data, len);
>  	if (!skb)
>  		return -ENOMEM;
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
> index 87db9498dea4..99f931c08da9 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> @@ -1383,6 +1383,9 @@ static inline int
>  mt76_mcu_send_msg(struct mt76_dev *dev, int cmd, const void *data, int len,
>  		  bool wait_resp)
>  {
> +	if (dev->mcu_ops->mcu_send_msg)
> +		return dev->mcu_ops->mcu_send_msg(dev, cmd, data, len, wait_resp);
> +
>  	return mt76_mcu_send_and_get_msg(dev, cmd, data, len, wait_resp, NULL);
>  }
>  

This patch seems correct since we run mcu_send_msg just for mt76x0 and mt76x2.
@Felix: what do you think?

Regards,
Lorenzo

> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]