dell-laptop: fix kbd_timeout handling

PATCH In review Dan Carpenter 4 participants 5 messages Dec 3, 2014 – Dec 4, 2014 1 message not in the archive x86 platform drivers

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 1417597233.14672.2.camel@Pali-Nokia-N900

Dan Carpenter

author

2014-12-03 10:01 UTC #1 lore raw

PATCH 1/1 dell-laptop: fix kbd_timeout handling

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 <[email protected]>

---
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')

drivers/platform/x86/dell-laptop.c +4 -2

index 46e9037..a311375 100644
@@ -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];

Darren Hart

2014-12-04 04:24 UTC #2 lore raw

Quoted 10 lines · 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:

Quoted 5 lines

	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 <[email protected]>

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,

Signature · 2 lines

Darren Hart
Intel Open Source Technology Center

Dan Carpenter

author

2014-12-04 08:30 UTC #3 lore raw

Quoted 2 lines · Darren Hart

Dan, any objection to Pali rolling this into another revision?

No problem, that's fine.

regards,
dan carpenter

Pali Rohár

2014-12-04 09:42 UTC #4 lore raw

In reply to Darren Hart

Quoted 19 lines · On Wednesday 03 December 2014 08:33:25 Darren Hart wrote:

Quoted 10 lines · 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
Quoted 7 lines
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 <[email protected]>

Quoted 8 lines

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?

Signature · 2 lines

Pali Rohár
[email protected]

Gabriele Mazzotta

2014-12-04 10:58 UTC #5 lore raw

Quoted 27 lines · On Thursday 04 December 2014 10:42:37 Pali Rohár wrote:

Quoted 26 lines · On Wednesday 03 December 2014 08:33:25 Darren Hart wrote:
Quoted 10 lines · 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
Quoted 7 lines
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 <[email protected]>
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.

Quoted 6 lines

Quoted 1 line
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.