[v2,1/5] HID: intel_ish-hid: fix potential uninitialized data usage

Message ID 20170518202144.3482304-2-arnd@arndb.de
State New
Headers show
Series
  • [v2,1/5] HID: intel_ish-hid: fix potential uninitialized data usage
Related show

Commit Message

Arnd Bergmann May 18, 2017, 8:21 p.m.
gcc points out an uninialized pointer dereference that could happen
if we ever get to recv_ishtp_cl_msg_dma() or recv_ishtp_cl_msg()
with an empty &dev->read_list:

drivers/hid/intel-ish-hid/ishtp/client.c: In function 'recv_ishtp_cl_msg_dma':
drivers/hid/intel-ish-hid/ishtp/client.c:1049:3: error: 'cl' may be used uninitialized in this function [-Werror=maybe-uninitialized]

The warning only appeared in very few randconfig builds, as the
spinlocks tend to prevent gcc from tracing the variables. I only
saw it in configurations that had neither SMP nor LOCKDEP enabled.

As we can see, we only enter the case if 'complete_rb' is non-NULL,
and then 'cl' is known to point to complete_rb->cl. Adding another
initialization to the same pointer is harmless here and makes it
clear to the compiler that the behavior is well-defined.

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

---
 drivers/hid/intel-ish-hid/ishtp/client.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Srinivas Pandruvada May 22, 2017, 7:17 p.m. | #1
Hi Arnd,
On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:
> gcc points out an uninialized pointer dereference that could happen

> if we ever get to recv_ishtp_cl_msg_dma() or recv_ishtp_cl_msg()

> with an empty &dev->read_list:

> 

> drivers/hid/intel-ish-hid/ishtp/client.c: In function

> 'recv_ishtp_cl_msg_dma':

> drivers/hid/intel-ish-hid/ishtp/client.c:1049:3: error: 'cl' may be

> used uninitialized in this function [-Werror=maybe-uninitialized]

> 

> The warning only appeared in very few randconfig builds, as the

> spinlocks tend to prevent gcc from tracing the variables. I only

> saw it in configurations that had neither SMP nor LOCKDEP enabled.

> 

> As we can see, we only enter the case if 'complete_rb' is non-NULL,

> and then 'cl' is known to point to complete_rb->cl. Adding another

> initialization to the same pointer is harmless here and makes it

> clear to the compiler that the behavior is well-defined.

> 

Did you get chance to test these changes on a platform with ISH?

Thanks,
Srinivas

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

> ---

>  drivers/hid/intel-ish-hid/ishtp/client.c | 2 ++

>  1 file changed, 2 insertions(+)

> 

> diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c

> b/drivers/hid/intel-ish-hid/ishtp/client.c

> index aad61328f282..78d393e616a4 100644

> --- a/drivers/hid/intel-ish-hid/ishtp/client.c

> +++ b/drivers/hid/intel-ish-hid/ishtp/client.c

> @@ -925,6 +925,7 @@ void recv_ishtp_cl_msg(struct ishtp_device *dev,

>  	}

>  

>  	if (complete_rb) {

> +		cl = complete_rb->cl;

>  		getnstimeofday(&cl->ts_rx);

>  		++cl->recv_msg_cnt_ipc;

>  		ishtp_cl_read_complete(complete_rb);

> @@ -1045,6 +1046,7 @@ void recv_ishtp_cl_msg_dma(struct ishtp_device

> *dev, void *msg,

>  	}

>  

>  	if (complete_rb) {

> +		cl = complete_rb->cl;

>  		getnstimeofday(&cl->ts_rx);

>  		++cl->recv_msg_cnt_dma;

>  		ishtp_cl_read_complete(complete_rb);

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann May 22, 2017, 9:33 p.m. | #2
On Mon, May 22, 2017 at 9:17 PM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> Hi Arnd,

> On Thu, 2017-05-18 at 22:21 +0200, Arnd Bergmann wrote:

>> gcc points out an uninialized pointer dereference that could happen

>> if we ever get to recv_ishtp_cl_msg_dma() or recv_ishtp_cl_msg()

>> with an empty &dev->read_list:

>>

>> drivers/hid/intel-ish-hid/ishtp/client.c: In function

>> 'recv_ishtp_cl_msg_dma':

>> drivers/hid/intel-ish-hid/ishtp/client.c:1049:3: error: 'cl' may be

>> used uninitialized in this function [-Werror=maybe-uninitialized]

>>

>> The warning only appeared in very few randconfig builds, as the

>> spinlocks tend to prevent gcc from tracing the variables. I only

>> saw it in configurations that had neither SMP nor LOCKDEP enabled.

>>

>> As we can see, we only enter the case if 'complete_rb' is non-NULL,

>> and then 'cl' is known to point to complete_rb->cl. Adding another

>> initialization to the same pointer is harmless here and makes it

>> clear to the compiler that the behavior is well-defined.

>>

> Did you get chance to test these changes on a platform with ISH?


No, I only build-tested it and though about the fix carefully.

        Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/hid/intel-ish-hid/ishtp/client.c b/drivers/hid/intel-ish-hid/ishtp/client.c
index aad61328f282..78d393e616a4 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client.c
@@ -925,6 +925,7 @@  void recv_ishtp_cl_msg(struct ishtp_device *dev,
 	}
 
 	if (complete_rb) {
+		cl = complete_rb->cl;
 		getnstimeofday(&cl->ts_rx);
 		++cl->recv_msg_cnt_ipc;
 		ishtp_cl_read_complete(complete_rb);
@@ -1045,6 +1046,7 @@  void recv_ishtp_cl_msg_dma(struct ishtp_device *dev, void *msg,
 	}
 
 	if (complete_rb) {
+		cl = complete_rb->cl;
 		getnstimeofday(&cl->ts_rx);
 		++cl->recv_msg_cnt_dma;
 		ishtp_cl_read_complete(complete_rb);