[PATCH] lis3lv02d: avoid divide by zero due to unchecked register read

[PATCH] lis3lv02d: avoid divide by zero due to unchecked register read

[Christian Lamparter]

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());

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 <chunkeey@googlemail.com>
---
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
---
diff --git a/drivers/misc/lis3lv02d/lis3lv02d.c b/drivers/misc/lis3lv02d/lis3lv02d.c
index b928bc1..96102f0 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d.c
+++ b/drivers/misc/lis3lv02d/lis3lv02d.c
@@ -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)

Re: [PATCH] lis3lv02d: avoid divide by zero due to unchecked register read

[Éric Piel]

Op 16-05-11 00:46, Christian Lamparter schreef:
> 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());
>
> 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



> Signed-off-by: Christian Lamparter<chunkeey@googlemail.com>
> ---
> 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
> ---
> diff --git a/drivers/misc/lis3lv02d/lis3lv02d.c b/drivers/misc/lis3lv02d/lis3lv02d.c
> index b928bc1..96102f0 100644
> --- a/drivers/misc/lis3lv02d/lis3lv02d.c
> +++ b/drivers/misc/lis3lv02d/lis3lv02d.c
> @@ -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)

Re: [RFC v2] lis3lv02d: avoid divide by zero due to unchecked register read

[Christian Lamparter]

On Monday 16 May 2011 13:16:46 Éric Piel wrote:
> Op 16-05-11 00:46, Christian Lamparter schreef:
> > 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() ).
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?].

> 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
---
diff --git a/drivers/misc/lis3lv02d/lis3lv02d.c b/drivers/misc/lis3lv02d/lis3lv02d.c
index b928bc1..60539c8 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d.c
+++ b/drivers/misc/lis3lv02d/lis3lv02d.c
@@ -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);
diff --git a/drivers/misc/lis3lv02d/lis3lv02d.h b/drivers/misc/lis3lv02d/lis3lv02d.h
index a193958..57c64bb 100644
--- a/drivers/misc/lis3lv02d/lis3lv02d.h
+++ b/drivers/misc/lis3lv02d/lis3lv02d.h
@@ -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;
diff --git a/drivers/platform/x86/hp_accel.c b/drivers/platform/x86/hp_accel.c
index 1b52d00..891e71f 100644
--- a/drivers/platform/x86/hp_accel.c
+++ b/drivers/platform/x86/hp_accel.c
@@ -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

Re: [RFC v2] lis3lv02d: avoid divide by zero due to unchecked register read

[Éric Piel]

Op 16-05-11 23:36, Christian Lamparter schreef:
> On Monday 16 May 2011 13:16:46 Éric Piel wrote:
>> Op 16-05-11 00:46, Christian Lamparter schreef:
>>>  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() ).
> 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?

>
> 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!

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

Re: [RFC v2] lis3lv02d: avoid divide by zero due to unchecked register read

[Christian Lamparter]

On Tuesday 17 May 2011 23:46:00 Éric Piel wrote:
> Op 16-05-11 23:36, Christian Lamparter schreef:
> > On Monday 16 May 2011 13:16:46 Éric Piel wrote:
> >> Op 16-05-11 00:46, Christian Lamparter schreef:
> >>>  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() ).
> > 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.

> 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".

> 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

Re: [RFC v2] lis3lv02d: avoid divide by zero due to unchecked register read

[Éric Piel]

Op 18-05-11 17:47, Christian Lamparter schreef:
> On Tuesday 17 May 2011 23:46:00 Éric Piel wrote:
>> Op 16-05-11 23:36, Christian Lamparter schreef:
>>> 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.
> 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.
>
>> 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.


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

Re: [RFC v2] lis3lv02d: avoid divide by zero due to unchecked register read

[Christian Lamparter]

On Wednesday 18 May 2011 17:56:18 Éric Piel wrote:
> Op 18-05-11 17:47, Christian Lamparter schreef:
> > On Tuesday 17 May 2011 23:46:00 Éric Piel wrote:
> >> Op 16-05-11 23:36, Christian Lamparter schreef:
> >>> 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.
> > 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.
> >
> >> 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.
> 
> >> 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