Re: [patch] dell-laptop: fix kbd_timeout handling

Re: [patch] dell-laptop: fix kbd_timeout handling

[Darren Hart]

On Wed, Dec 03, 2014 at 01:01:59PM +0300, Dan Carpenter wrote:
> The original code had a static checker warning:
> 
> 	drivers/platform/x86/dell-laptop.c:1389 kbd_led_timeout_store()
> 	warn: this array is probably non-NULL. 'quirks->kbd_timeouts'
> 
> This warning does indicate a bug.  I have added a .needs_kbd_timeouts
> flag which is true if the .kbd_timeouts[] array has been declared.
> Otherwise, in the original code the quirk_dell_vostro_v130 struct didn't
> have .kbd_timeouts[] declared so the loop:
> 
> 	for (i = 0; quirks->kbd_timeouts[i] != -1; i++) {
> 
> Would have read beyond the end of the struct with unpredictable results.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>


Thanks Dan,

Pali, any objections?

Dan, any objection to Pali rolling this into another revision? No point in
introducing the bug to next and mainline.

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

[patch] dell-laptop: fix kbd_timeout handling

[Dan Carpenter]

The original code had a static checker warning:

	drivers/platform/x86/dell-laptop.c:1389 kbd_led_timeout_store()
	warn: this array is probably non-NULL. 'quirks->kbd_timeouts'

This warning does indicate a bug.  I have added a .needs_kbd_timeouts
flag which is true if the .kbd_timeouts[] array has been declared.
Otherwise, in the original code the quirk_dell_vostro_v130 struct didn't
have .kbd_timeouts[] declared so the loop:

	for (i = 0; quirks->kbd_timeouts[i] != -1; i++) {

Would have read beyond the end of the struct with unpredictable results.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This fixes a patch in Darren Hart's tree and it may not have yet reached
linux-next.  The patch is 167e24aa8adf ('platform: x86: dell-laptop: Add
support for keyboard backlight')

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 46e9037..a311375 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -72,6 +72,7 @@ struct calling_interface_structure {
 struct quirk_entry {
 	u8 touchpad_led;
 
+	int needs_kbd_timeouts;
 	/* Ordered list of timeouts expressed in seconds.
 	 * The list must end with -1 */
 	int kbd_timeouts[];
@@ -90,6 +91,7 @@ static int __init dmi_matched(const struct dmi_system_id *dmi)
 }
 
 static struct quirk_entry quirk_dell_xps13_9333 = {
+	.needs_kbd_timeouts = 1,
 	.kbd_timeouts = { 0, 5, 15, 60, 5*60, 15*60, -1 },
 };
 
@@ -1386,7 +1388,7 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
 		return -EINVAL;
 	}
 
-	if (quirks && quirks->kbd_timeouts)
+	if (quirks && quirks->needs_kbd_timeouts)
 		convert = true;
 
 	if (convert) {
@@ -1401,7 +1403,7 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
 			unit = KBD_TIMEOUT_SECONDS;
 		}
 
-		if (quirks && quirks->kbd_timeouts) {
+		if (quirks && quirks->needs_kbd_timeouts) {
 			for (i = 0; quirks->kbd_timeouts[i] != -1; i++) {
 				if (value <= quirks->kbd_timeouts[i]) {
 					value = quirks->kbd_timeouts[i];

Re: [patch] dell-laptop: fix kbd_timeout handling

[Dan Carpenter]

On Tue, Dec 02, 2014 at 11:33:25PM -0800, Darren Hart wrote:
>
> Dan, any objection to Pali rolling this into another revision?

No problem, that's fine.

regards,
dan carpenter

Re: [patch] dell-laptop: fix kbd_timeout handling

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1191 bytes --]

On Wednesday 03 December 2014 08:33:25 Darren Hart wrote:
> On Wed, Dec 03, 2014 at 01:01:59PM +0300, Dan Carpenter wrote:
> > The original code had a static checker warning:
> > 	drivers/platform/x86/dell-laptop.c:1389
> > 	kbd_led_timeout_store() warn: this array is probably
> > 	non-NULL. 'quirks->kbd_timeouts'
> > 
> > This warning does indicate a bug.  I have added a
> > .needs_kbd_timeouts flag which is true if the
> > .kbd_timeouts[] array has been declared. Otherwise, in the
> > original code the quirk_dell_vostro_v130 struct didn't
> > 
> > have .kbd_timeouts[] declared so the loop:
> > 	for (i = 0; quirks->kbd_timeouts[i] != -1; i++) {
> > 
> > Would have read beyond the end of the struct with
> > unpredictable results.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Thanks Dan,
> 
> Pali, any objections?
> 
> Dan, any objection to Pali rolling this into another revision?
> No point in introducing the bug to next and mainline.
> 
> Thanks,

No problem from my side. This quirks code was done by Gabriele to 
fix BIOS problems on some laptops.

Gabriele, any objections?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [patch] dell-laptop: fix kbd_timeout handling

[Gabriele Mazzotta]

On Thursday 04 December 2014 10:42:37 Pali Rohár wrote:
> On Wednesday 03 December 2014 08:33:25 Darren Hart wrote:
> > On Wed, Dec 03, 2014 at 01:01:59PM +0300, Dan Carpenter wrote:
> > > The original code had a static checker warning:
> > > 	drivers/platform/x86/dell-laptop.c:1389
> > > 	kbd_led_timeout_store() warn: this array is probably
> > > 	non-NULL. 'quirks->kbd_timeouts'
> > > 
> > > This warning does indicate a bug.  I have added a
> > > .needs_kbd_timeouts flag which is true if the
> > > .kbd_timeouts[] array has been declared. Otherwise, in the
> > > original code the quirk_dell_vostro_v130 struct didn't
> > > 
> > > have .kbd_timeouts[] declared so the loop:
> > > 	for (i = 0; quirks->kbd_timeouts[i] != -1; i++) {
> > > 
> > > Would have read beyond the end of the struct with
> > > unpredictable results.
> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > Thanks Dan,
> > 
> > Pali, any objections?
> > 
> > Dan, any objection to Pali rolling this into another revision?
> > No point in introducing the bug to next and mainline.
> > 
> > Thanks,
> 
> No problem from my side. This quirks code was done by Gabriele to
> fix BIOS problems on some laptops.
> 
> Gabriele, any objections?

No objections.

Gabriele.