qtnfmac: avoid uninitialized variable access

Message ID 20181009155757.494212-1-arnd@arndb.de
State New
Headers show
Series
  • qtnfmac: avoid uninitialized variable access
Related show

Commit Message

Arnd Bergmann Oct. 9, 2018, 3:57 p.m.
When qtnf_trans_send_cmd_with_resp() fails, we have not yet initialized
'resp', as pointed out by a valid gcc warning:

drivers/net/wireless/quantenna/qtnfmac/commands.c: In function 'qtnf_cmd_send_with_reply':
drivers/net/wireless/quantenna/qtnfmac/commands.c:133:54: error: 'resp' may be used uninitialized in this function [-Werror=maybe-uninitialized]

Since 'resp_skb' is also not set here, we can skip all further
processing and just print the warning and return the failure code.

Fixes: c6ed298ffe09 ("qtnfmac: cleanup and unify command error handling")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 drivers/net/wireless/quantenna/qtnfmac/commands.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.18.0

Comments

Sergey Matyukevich Oct. 9, 2018, 8:25 p.m. | #1
Hello Arnd,

> When qtnf_trans_send_cmd_with_resp() fails, we have not yet initialized

> 'resp', as pointed out by a valid gcc warning:

> 

> drivers/net/wireless/quantenna/qtnfmac/commands.c: In function 'qtnf_cmd_send_with_reply':

> drivers/net/wireless/quantenna/qtnfmac/commands.c:133:54: error: 'resp' may be used uninitialized in this function [-Werror=maybe-uninitialized]

> 

> Since 'resp_skb' is also not set here, we can skip all further

> processing and just print the warning and return the failure code.

> 

> Fixes: c6ed298ffe09 ("qtnfmac: cleanup and unify command error handling")

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>


Thanks for the patch! And for reminding me that I forgot to enable
gcc warnings in CI builds in addition to sparse checks.

Reviewed-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>


Regards,
Sergey
Sergey Matyukevich Oct. 10, 2018, 2:56 p.m. | #2
> > When qtnf_trans_send_cmd_with_resp() fails, we have not yet initialized

> > 'resp', as pointed out by a valid gcc warning:

> >

> > drivers/net/wireless/quantenna/qtnfmac/commands.c: In function 'qtnf_cmd_send_with_reply':

> > drivers/net/wireless/quantenna/qtnfmac/commands.c:133:54: error: 'resp' may be used uninitialized in this function [-Werror=maybe-uninitialized]

> >

> > Since 'resp_skb' is also not set here, we can skip all further

> > processing and just print the warning and return the failure code.

> >

> > Fixes: c6ed298ffe09 ("qtnfmac: cleanup and unify command error handling")

> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> 

> Thanks for the patch! And for reminding me that I forgot to enable

> gcc warnings in CI builds in addition to sparse checks.

> 

> Reviewed-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>


Hi Kalle,

Could you please hold back applying this patch for now. We have got
another report for the same function, this time static analysis tool
warning. It looks like the patch from Arnd does not cover both cases.
So we will take a closer look and send a combined fix later.

Regards,
Sergey
Kalle Valo Oct. 11, 2018, 8:08 a.m. | #3
Sergey Matyukevich <sergey.matyukevich.os@quantenna.com> writes:

>> > When qtnf_trans_send_cmd_with_resp() fails, we have not yet initialized

>> > 'resp', as pointed out by a valid gcc warning:

>> >

>> > drivers/net/wireless/quantenna/qtnfmac/commands.c: In function

>> > 'qtnf_cmd_send_with_reply':

>> > drivers/net/wireless/quantenna/qtnfmac/commands.c:133:54: error:

>> > 'resp' may be used uninitialized in this function

>> > [-Werror=maybe-uninitialized]

>> >

>> > Since 'resp_skb' is also not set here, we can skip all further

>> > processing and just print the warning and return the failure code.

>> >

>> > Fixes: c6ed298ffe09 ("qtnfmac: cleanup and unify command error handling")

>> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>

>> 

>> Thanks for the patch! And for reminding me that I forgot to enable

>> gcc warnings in CI builds in addition to sparse checks.

>> 

>> Reviewed-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>

>

> Hi Kalle,

>

> Could you please hold back applying this patch for now. We have got

> another report for the same function, this time static analysis tool

> warning. It looks like the patch from Arnd does not cover both cases.

> So we will take a closer look and send a combined fix later.


Ok, I'll drop this from my queue. Please resend whatever patch I need to
apply.

-- 
Kalle Valo

Patch

diff --git a/drivers/net/wireless/quantenna/qtnfmac/commands.c b/drivers/net/wireless/quantenna/qtnfmac/commands.c
index bfdc1ad30c13..9b211459684a 100644
--- a/drivers/net/wireless/quantenna/qtnfmac/commands.c
+++ b/drivers/net/wireless/quantenna/qtnfmac/commands.c
@@ -111,7 +111,7 @@  static int qtnf_cmd_send_with_reply(struct qtnf_bus *bus,
 
 	ret = qtnf_trans_send_cmd_with_resp(bus, cmd_skb, &resp_skb);
 	if (ret)
-		goto out;
+		goto out_noresp;
 
 	resp = (const struct qlink_resp *)resp_skb->data;
 	ret = qtnf_cmd_check_reply_header(resp, cmd_id, mac_id, vif_id,
@@ -132,6 +132,7 @@  static int qtnf_cmd_send_with_reply(struct qtnf_bus *bus,
 	if (!ret && resp)
 		return qtnf_cmd_resp_result_decode(le16_to_cpu(resp->result));
 
+out_noresp:
 	pr_warn("VIF%u.%u: cmd 0x%.4X failed: %d\n",
 		mac_id, vif_id, le16_to_cpu(cmd->cmd_id), ret);