lis3lv02d: avoid divide by zero due to unchecked register read

PATCH In review Christian Lamparter 2 participants 7 messages May 15, 2011 – May 18, 2011

Subsystems (3)

Char and misc drivers x86 platform drivers LIS3LV02D accelerometer driver

lore

Share & tools

lore

Christian Lamparter

author

2011-05-15 22:46 UTC #0 lore raw

PATCH 1/1 lis3lv02d: avoid divide by zero due to unchecked register read

After an "unexpected" reboot, I found this Oops in my logs:

divide error: 0000 [#1] PREEMPT SMP
CPU 0

Modules linked in: lis3lv02d hp_wmi input_polldev [...]
Pid: 390, comm: modprobe Tainted: G         C  2.6.39-rc7-wl+ 
RIP: 0010:[<ffffffffa014b427>]  [<ffffffffa014b427>]
		 lis3lv02d_poweron+0x4e/0x94 [lis3lv02d]
RSP: 0018:ffff8801d6407cf8  EFLAGS: 00010246
RAX: 0000000000000bb8 RBX: ffffffffa014e000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffea00066e4708 RDI: ffff8801df002700
RBP: ffff8801d6407d18 R08: ffffea00066c5a30 R09: ffffffff812498c9
R10: ffff8801d7bfcea0 R11: ffff8801d7bfce10 R12: 0000000000000bb8
R13: 00000000ffffffda R14: ffffffffa0154120 R15: ffffffffa0154030
FS:  00007fc0705db700(0000) GS:ffff8801dfa00000(0000) knlGS:0
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00007f33549174f0 CR3: 00000001d65c9000 CR4: 00000000000406f0
Process modprobe (pid: 390, threadinfo ffff8801d6406000, task ffff8801d6b40000)
Stack:
 ffffffffa0154120 62ffffffa0154030 ffffffffa014e000 00000000ffffffea
 ffff8801d6407d58 ffffffffa014bcc1 0000000000000000 0000000000000048
 ffff8801d8bae800 00000000ffffffea 00000000ffffffda ffffffffa0154120
Call Trace:
 [<ffffffffa014bcc1>] lis3lv02d_init_device+0x1ce/0x496 [lis3lv02d]
 [<ffffffffa01522ff>] lis3lv02d_add+0x10f/0x17c [hp_accel]
 [<ffffffff81233e11>] acpi_device_probe+0x49/0x117
[...]
Code: 3a 75 06 80 4d ef 50 eb 04 80 4d ef 40 0f b6 55 ef be 21

00 00 00 48 89 df ff 53 18 44 8b 63 6c e8 3e fc ff ff 89 c1 44 89 e0 99 <f7> f9 89 c7 e8 93 82 ef e0 48 83 7b 30 00 74 2d 45 31 e4 80 7b
RIP [<ffffffffa014b427>] lis3lv02d_poweron+0x4e/0x94 [lis3lv02d]
RSP <ffff8801d6407cf8>

Quoted 1 line

From my POV, it looks like the hardware is not working as expected

and returns a bogus data rate. The driver doesn't check the result and directly uses it as some sort of divisor in some places:

msleep(lis3->pwron_delay / lis3lv02d_get_odr());

Under this circumstances, this could very well cause the "divide by zero" exception from above.

For now, I fixed it the easiest and most obvious way:
Check if the result is sane and if it isn't use a sane default instead. I went for "100" in the latter case, simply because /sys/devices/platform/lis3lv02d/rate returns it on a successful boot.

Signed-off-by Christian Lamparter <[email protected]>

---
Oh, please keep the CC.

Note: This patch is a bit experimental, I can't get the device to fail reliable so even with the patch, it might fail in a different place.

Regards,
Chr
---

drivers/misc/lis3lv02d/lis3lv02d.c +8 -2

index b928bc1..96102f0 100644
@@ -198,12 +198,18 @@ static int lis3_3dc_rates[16] = {0, 1, 10, 25, 50, 100, 200, 400, 1600, 5000};
 static int lis3lv02d_get_odr(void)
 {
 	u8 ctrl;
-	int shift;
+	int shift, res;
 
 	lis3_dev.read(&lis3_dev, CTRL_REG1, &ctrl);
 	ctrl &= lis3_dev.odr_mask;
 	shift = ffs(lis3_dev.odr_mask) - 1;
-	return lis3_dev.odrs[(ctrl >> shift)];
+	res = lis3_dev.odrs[(ctrl >> shift)];
+
+	/*
+	 * because the result is used as a divisor, it must
+	 * not be a zero.
+	 */
+	return (res != 0) ? res : 100;
 }
 
 static int lis3lv02d_set_odr(int rate)

Éric Piel

maintainer

2011-05-16 11:41 UTC #1 lore raw

Op 16-05-11 00:46, Christian Lamparter schreef:

Quoted 40 lines

After an "unexpected" reboot, I found this Oops in my logs:

divide error: 0000 [#1] PREEMPT SMP
CPU 0

Modules linked in: lis3lv02d hp_wmi input_polldev [...]
Pid: 390, comm: modprobe Tainted: G         C  2.6.39-rc7-wl+
RIP: 0010:[<ffffffffa014b427>]  [<ffffffffa014b427>]
		 lis3lv02d_poweron+0x4e/0x94 [lis3lv02d]
RSP: 0018:ffff8801d6407cf8  EFLAGS: 00010246
RAX: 0000000000000bb8 RBX: ffffffffa014e000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffea00066e4708 RDI: ffff8801df002700
RBP: ffff8801d6407d18 R08: ffffea00066c5a30 R09: ffffffff812498c9
R10: ffff8801d7bfcea0 R11: ffff8801d7bfce10 R12: 0000000000000bb8
R13: 00000000ffffffda R14: ffffffffa0154120 R15: ffffffffa0154030
FS:  00007fc0705db700(0000) GS:ffff8801dfa00000(0000) knlGS:0
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00007f33549174f0 CR3: 00000001d65c9000 CR4: 00000000000406f0
Process modprobe (pid: 390, threadinfo ffff8801d6406000, task ffff8801d6b40000)
Stack:
  ffffffffa0154120 62ffffffa0154030 ffffffffa014e000 00000000ffffffea
  ffff8801d6407d58 ffffffffa014bcc1 0000000000000000 0000000000000048
  ffff8801d8bae800 00000000ffffffea 00000000ffffffda ffffffffa0154120
Call Trace:
  [<ffffffffa014bcc1>] lis3lv02d_init_device+0x1ce/0x496 [lis3lv02d]
  [<ffffffffa01522ff>] lis3lv02d_add+0x10f/0x17c [hp_accel]
  [<ffffffff81233e11>] acpi_device_probe+0x49/0x117
[...]
Code: 3a 75 06 80 4d ef 50 eb 04 80 4d ef 40 0f b6 55 ef be 21

00 00 00 48 89 df ff 53 18 44 8b 63 6c e8 3e fc ff ff 89 c1 44 89 e0 99<f7> f9 89 c7 e8 93 82 ef e0 48 83 7b 30 00 74 2d 45 31 e4 80 7b
RIP [<ffffffffa014b427>] lis3lv02d_poweron+0x4e/0x94 [lis3lv02d]
RSP<ffff8801d6407cf8>

From my POV, it looks like the hardware is not working as expected
and returns a bogus data rate. The driver doesn't check the result and directly uses it as some sort of divisor in some places:

msleep(lis3->pwron_delay / lis3lv02d_get_odr());

Quoted 9 lines

Under this circumstances, this could very well cause the "divide by zero" exception from above.
For now, I fixed it the easiest and most obvious way:
Check if the result is sane and if it isn't use a sane default instead. I went for "100" in the latter case, simply because /sys/devices/platform/lis3lv02d/rate returns it on a successful boot.

Hi,
Good catch! We don't want to have a kernel oops just because a device is sending bad data...

However, I'd fix it a bit differently: let lis3lv02d_get_odr() return the raw data, and create a special function lis3lv02d_get_pwron_delay_ms() which does the "lis3->pwron_delay / lis3lv02d_get_odr()" with special handling for 0 (returning a large value and also sending a printk_once() ).

As you have noted, we might want to check other parts of the driver to validate the data from the device. So far, all the code considers that the device is flawless :-S

Cheers,
Éric

Quoted 36 lines

Signed-off-by Christian Lamparter<[email protected]>

---
Oh, please keep the CC.

Note: This patch is a bit experimental, I can't get the device to fail reliable so even with the patch, it might fail in a different place.

Regards,
Chr
---

drivers/misc/lis3lv02d/lis3lv02d.c +8 -2

index b928bc1..96102f0 100644
@@ -198,12 +198,18 @@ static int lis3_3dc_rates[16] = {0, 1, 10, 25, 50, 100, 200, 400, 1600, 5000};
  static int lis3lv02d_get_odr(void)
  {
  	u8 ctrl;
-	int shift;
+	int shift, res;
 
  	lis3_dev.read(&lis3_dev, CTRL_REG1,&ctrl);
  	ctrl&= lis3_dev.odr_mask;
  	shift = ffs(lis3_dev.odr_mask) - 1;
-	return lis3_dev.odrs[(ctrl>>  shift)];
+	res = lis3_dev.odrs[(ctrl>>  shift)];
+
+	/*
+	 * because the result is used as a divisor, it must
+	 * not be a zero.
+	 */
+	return (res != 0) ? res : 100;
  }
 
  static int lis3lv02d_set_odr(int rate)

Christian Lamparter

author

2011-05-16 21:36 UTC #2 lore raw

Quoted 7 lines · On Monday 16 May 2011 13:16:46 Éric Piel wrote:

Op 16-05-11 00:46, Christian Lamparter schreef:
Quoted 6 lines
From my POV, it looks like the hardware is not working as expected and returns a bogus data rate. The driver doesn't check the result and directly uses it as some sort of divisor in some places:
msleep(lis3->pwron_delay / lis3lv02d_get_odr());

Quoted 8 lines

Quoted 3 lines
Under this circumstances, this could very well cause the "divide by zero" exception from above.
However, I'd fix it a bit differently: let lis3lv02d_get_odr() return the raw data, and create a special function lis3lv02d_get_pwron_delay_ms() which does the "lis3->pwron_delay / lis3lv02d_get_odr()" with special handling for 0 (returning a large value and also sending a printk_once() ).

Do you know how "volatile" this data rate is? If it never changes [at least it doesn't here?] then why not read it once in init_device and store it in the device context?

Anyway, I updated the code... But instead of returning a "large value" I went for the -ENXIO to bail-out early, so now we won't continue if something went bad [after resume for instance?].

Quoted 3 lines

As you have noted, we might want to check other parts of the driver to validate the data from the device. So far, all the code considers that the device is flawless :-S

well:
CHECK drivers/misc/lis3lv02d/lis3lv02d.c
drivers/misc/lis3lv02d/lis3lv02d.c:170:52: warning: cast to restricted __le16

but this is a tiny tiny niggle (on x86 at least).
---
v1 -> v2
- Eric's comments
---

drivers/misc/lis3lv02d/lis3lv02d.c +30 -6

index b928bc1..60539c8 100644
@@ -206,6 +206,18 @@ static int lis3lv02d_get_odr(void)
 	return lis3_dev.odrs[(ctrl >> shift)];
 }
 
+static int lis3lv02d_get_pwron_wait(struct lis3lv02d *lis3)
+{
+	int div = lis3lv02d_get_odr();
+
+	if (WARN_ONCE(div == 0, "device returned spurious data"))
+		return -ENXIO;
+
+	/* LIS3 power on delay is quite long */
+	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
+	return 0;
+}
+
 static int lis3lv02d_set_odr(int rate)
 {
 	u8 ctrl;

@@ -266,7 +278,9 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
 
 	lis3->read(lis3, ctlreg, &reg);
 	lis3->write(lis3, ctlreg, (reg | selftest));
-	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
+	ret = lis3lv02d_get_pwron_wait(lis3);
+	if (ret)
+		goto fail;
 
 	/* Read directly to avoid axis remap */
 	x = lis3->read_data(lis3, OUTX);

@@ -275,7 +289,9 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
 
 	/* back to normal settings */
 	lis3->write(lis3, ctlreg, reg);
-	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
+	ret = lis3lv02d_get_pwron_wait(lis3);
+	if (ret)
+		goto fail;
 
 	results[0] = x - lis3->read_data(lis3, OUTX);
 	results[1] = y - lis3->read_data(lis3, OUTY);

@@ -363,8 +379,9 @@ void lis3lv02d_poweroff(struct lis3lv02d *lis3)
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_poweroff);
 
-void lis3lv02d_poweron(struct lis3lv02d *lis3)
+int lis3lv02d_poweron(struct lis3lv02d *lis3)
 {
+	int err;
 	u8 reg;
 
 	lis3->init(lis3);

@@ -382,11 +399,14 @@ void lis3lv02d_poweron(struct lis3lv02d *lis3)
 		reg |= CTRL2_BOOT_8B;
 	lis3->write(lis3, CTRL_REG2, reg);
 
-	/* LIS3 power on delay is quite long */
-	msleep(lis3->pwron_delay / lis3lv02d_get_odr());
+	err = lis3lv02d_get_pwron_wait(lis3);
+	if (err)
+		return err;
 
 	if (lis3->reg_ctrl)
 		lis3_context_restore(lis3);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(lis3lv02d_poweron);

@@ -926,7 +946,11 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
 	atomic_set(&dev->wake_thread, 0);
 
 	lis3lv02d_add_fs(dev);
-	lis3lv02d_poweron(dev);
+	err = lis3lv02d_poweron(dev);
+	if (err) {
+		lis3lv02d_remove_fs(dev);
+		return err;
+	}
 
 	if (dev->pm_dev) {
 		pm_runtime_set_active(dev->pm_dev);

drivers/misc/lis3lv02d/lis3lv02d.h +1 -1

index a193958..57c64bb 100644
@@ -285,7 +285,7 @@ int lis3lv02d_init_device(struct lis3lv02d *lis3);
 int lis3lv02d_joystick_enable(void);
 void lis3lv02d_joystick_disable(void);
 void lis3lv02d_poweroff(struct lis3lv02d *lis3);
-void lis3lv02d_poweron(struct lis3lv02d *lis3);
+int lis3lv02d_poweron(struct lis3lv02d *lis3);
 int lis3lv02d_remove_fs(struct lis3lv02d *lis3);
 
 extern struct lis3lv02d lis3_dev;

drivers/platform/x86/hp_accel.c +1 -2

index 1b52d00..891e71f 100644
@@ -354,8 +354,7 @@ static int lis3lv02d_suspend(struct acpi_device *device, pm_message_t state)
 
 static int lis3lv02d_resume(struct acpi_device *device)
 {
-	lis3lv02d_poweron(&lis3_dev);
-	return 0;
+	return lis3lv02d_poweron(&lis3_dev);
 }
 #else
 #define lis3lv02d_suspend NULL

Éric Piel

maintainer

2011-05-17 21:46 UTC #3 lore raw

Op 16-05-11 23:36, Christian Lamparter schreef:

Quoted 11 lines · On Monday 16 May 2011 13:16:46 Éric Piel wrote:

Quoted 10 lines
Op 16-05-11 00:46, Christian Lamparter schreef:
Quoted 9 lines
From my POV, it looks like the hardware is not working as expected
and returns a bogus data rate. The driver doesn't check the result and directly uses it as some sort of divisor in some places:
msleep(lis3->pwron_delay / lis3lv02d_get_odr());
Under this circumstances, this could very well cause the "divide by zero" exception from above.

Quoted 8 lines

Quoted 5 lines
However, I'd fix it a bit differently: let lis3lv02d_get_odr() return the raw data, and create a special function lis3lv02d_get_pwron_delay_ms() which does the "lis3->pwron_delay / lis3lv02d_get_odr()" with special handling for 0 (returning a large value and also sending a printk_once() ).
Do you know how "volatile" this data rate is? If it never changes [at least it doesn't here?] then why not read it once in init_device and store it in the device context?

It is not normally changing, normally it is set just at init/unsuspend (where the bios can also interfere sometimes) and when the user changes it. So definitely within the same function it's not going to suddenly change. We could avoid calculating/checking it twice in lis3lv02d_selftest(). Care to do a third version with this little clean up?

Quoted 4 lines

Anyway, I updated the code... But instead of returning a "large value" I went for the -ENXIO to bail-out early, so now we won't continue if something went bad [after resume for instance?].

Sounds even better than using a conservative value!

Quoted 7 lines

Quoted 4 lines
As you have noted, we might want to check other parts of the driver to validate the data from the device. So far, all the code considers that the device is flawless :-S
well:
CHECK drivers/misc/lis3lv02d/lis3lv02d.c
drivers/misc/lis3lv02d/lis3lv02d.c:170:52: warning: cast to restricted __le16

This is not introduced by your patch, right? So it's fine for now :-)

Thanks,
Eric

Christian Lamparter

author

2011-05-18 15:47 UTC #4 lore raw

Quoted 17 lines · On Tuesday 17 May 2011 23:46:00 Éric Piel wrote:

Op 16-05-11 23:36, Christian Lamparter schreef:
Quoted 16 lines · On Monday 16 May 2011 13:16:46 Éric Piel wrote:
Quoted 15 lines
Op 16-05-11 00:46, Christian Lamparter schreef:
Quoted 9 lines
From my POV, it looks like the hardware is not working as expected
and returns a bogus data rate. The driver doesn't check the result and directly uses it as some sort of divisor in some places:
msleep(lis3->pwron_delay / lis3lv02d_get_odr());
Under this circumstances, this could very well cause the "divide by zero" exception from above.
However, I'd fix it a bit differently: let lis3lv02d_get_odr() return the raw data, and create a special function lis3lv02d_get_pwron_delay_ms() which does the "lis3->pwron_delay / lis3lv02d_get_odr()" with special handling for 0 (returning a large value and also sending a printk_once() ).

Quoted 6 lines

Quoted 3 lines
Do you know how "volatile" this data rate is? If it never changes [at least it doesn't here?] then why not read it once in init_device and store it in the device context?
It is not normally changing, normally it is set just at init/unsuspend (where the bios can also interfere sometimes) and when the user changes it.

Uh, "bios can also interfere"... this sounds very bad. At least for my x41t the bios doesn't care about hdaps once the OS is running.

Quoted 2 lines

So definitely within the same function it's not going to suddenly change.

a SMM can happen at any time and if a faulty BIOS [likely, since I got a new laptop] is what caused the crash, I wouldn't bet on "const within a function context".

Quoted 2 lines

We could avoid calculating/checking it twice in lis3lv02d_selftest(). Care to do a third version with this little clean up?

I have my doubts, but ok if you say so... Just one thing: need to do some Q&A on the code above, I haven't tested it extensively yet.

Regards,
Chr

Éric Piel

maintainer

2011-05-18 15:56 UTC #5 lore raw

Op 18-05-11 17:47, Christian Lamparter schreef:

Quoted 3 lines · On Tuesday 17 May 2011 23:46:00 Éric Piel wrote:

Quoted 2 lines
Op 16-05-11 23:36, Christian Lamparter schreef:
Quoted 1 line · On Monday 16 May 2011 13:16:46 Éric Piel wrote:

Quoted 6 lines

Quoted 6 lines
Quoted 3 lines
Do you know how "volatile" this data rate is? If it never changes [at least it doesn't here?] then why not read it once in init_device and store it in the device context?
It is not normally changing, normally it is set just at init/unsuspend (where the bios can also interfere sometimes) and when the user changes it.

Quoted 8 lines

Uh, "bios can also interfere"... this sounds very bad. At least for my x41t the bios doesn't care about hdaps once the OS is running.
Quoted 2 lines
So definitely within the same function it's not going to suddenly change.
a SMM can happen at any time and if a faulty BIOS [likely, since I got a new laptop] is what caused the crash, I wouldn't bet on "const within a function context".

Yes, at least on the HP laptop I have the bios enjoys re-initialising the hardware at some default value whenever suspend/resume happens.

Quoted 4 lines

Quoted 2 lines
We could avoid calculating/checking it twice in lis3lv02d_selftest(). Care to do a third version with this little clean up?
I have my doubts, but ok if you say so... Just one thing: need to do some Q&A on the code above, I haven't tested it extensively yet.

I didn't know about the SMM... well, then let's keep the code as is.

I'll try to test it on my laptop in the coming days. On which hardware have you tested it?

Éric

Christian Lamparter

author

2011-05-18 16:08 UTC #6 lore raw

Quoted 21 lines · On Wednesday 18 May 2011 17:56:18 Éric Piel wrote:

Op 18-05-11 17:47, Christian Lamparter schreef:
Quoted 9 lines · On Tuesday 17 May 2011 23:46:00 Éric Piel wrote:
Quoted 8 lines
Op 16-05-11 23:36, Christian Lamparter schreef:
Quoted 4 lines · On Monday 16 May 2011 13:16:46 Éric Piel wrote:
Do you know how "volatile" this data rate is? If it never changes [at least it doesn't here?] then why not read it once in init_device and store it in the device context?
It is not normally changing, normally it is set just at init/unsuspend (where the bios can also interfere sometimes) and when the user changes it.
Quoted 8 lines
Uh, "bios can also interfere"... this sounds very bad. At least for my x41t the bios doesn't care about hdaps once the OS is running.
Quoted 2 lines
So definitely within the same function it's not going to suddenly change.
a SMM can happen at any time and if a faulty BIOS [likely, since I got a new laptop] is what caused the crash, I wouldn't bet on "const within a function context".
Yes, at least on the HP laptop I have the bios enjoys re-initialising the hardware at some default value whenever suspend/resume happens.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
We could avoid calculating/checking it twice in lis3lv02d_selftest(). Care to do a third version with this little clean up?
I have my doubts, but ok if you say so... Just one thing: need to do some Q&A on the code above, I haven't tested it extensively yet.
I didn't know about the SMM... well, then let's keep the code as is.
I'll try to test it on my laptop in the coming days. On which hardware have you tested it?

I've a dv6-6003eg, it's fairly new.

Regards,
Chr