[PATCH 1/1] integer overflow issue in 'appletouch' driver

[PATCH 1/1] integer overflow issue in 'appletouch' driver

[Vadim Zaliva]

This small patch is fixing an integer overflow issue in 'appletouch' driver.

In particular, reading data from Geyser 2 touchpads used on post Oct
2005 Apple PowerBooks the driver was casting X and Y coordinates
values to 'signed char'. Testing on one of such PowerBooks I have
noticed that touchpad always generates positive values, but some of
them are greater that 127, and thus, when cast to 'signed char' being
interpreted as a negative.

Such bigger values have been observed infrequently, closer to the
edges of a touchpad, so the problem was not very visible. 
Nevertheless, the patch would potentially improve touchpad
driver accuracy.


diff -uNr linux-source-2.6.31.orig/drivers/input/mouse/appletouch.c linux-source-2.6.31/drivers/input/mouse/appletouch.c
--- linux-source-2.6.31.orig/drivers/input/mouse/appletouch.c	2009-09-09 15:13:59.000000000 -0700
+++ linux-source-2.6.31/drivers/input/mouse/appletouch.c	2010-03-05 11:05:11.921394055 -0800
@@ -205,8 +205,8 @@
 	bool			overflow_warned;
 	int			x_old;		/* last reported x/y, */
 	int			y_old;		/* used for smoothing */
-	signed char		xy_cur[ATP_XSENSORS + ATP_YSENSORS];
-	signed char		xy_old[ATP_XSENSORS + ATP_YSENSORS];
+	u8		    xy_cur[ATP_XSENSORS + ATP_YSENSORS];
+	u8		    xy_old[ATP_XSENSORS + ATP_YSENSORS];
 	int			xy_acc[ATP_XSENSORS + ATP_YSENSORS];
 	int			idlecount;	/* number of empty packets */
 	struct work_struct	work;
@@ -531,7 +531,7 @@
 
 	for (i = 0; i < ATP_XSENSORS + ATP_YSENSORS; i++) {
 		/* accumulate the change */
-		signed char change = dev->xy_old[i] - dev->xy_cur[i];
+		int change = dev->xy_old[i] - dev->xy_cur[i];
 		dev->xy_acc[i] -= change;
 
 		/* prevent down drifting */

Re: [PATCH 1/1] integer overflow issue in 'appletouch' driver

[Johannes Berg]

On Fri, 2010-03-05 at 11:47 -0800, Vadim Zaliva wrote:
> This small patch is fixing an integer overflow issue in 'appletouch' driver.
> 
> In particular, reading data from Geyser 2 touchpads used on post Oct
> 2005 Apple PowerBooks the driver was casting X and Y coordinates
> values to 'signed char'. Testing on one of such PowerBooks I have
> noticed that touchpad always generates positive values, but some of
> them are greater that 127, and thus, when cast to 'signed char' being
> interpreted as a negative.
> 
> Such bigger values have been observed infrequently, closer to the
> edges of a touchpad, so the problem was not very visible. 
> Nevertheless, the patch would potentially improve touchpad
> driver accuracy.

Hmm, you're probably right, and I cannot test with my powerbook as I'm
about to leave on travel and it's at my parents.

johannes

Re: [PATCH 1/1] integer overflow issue in 'appletouch' driver

[Vadim Zaliva]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

On Mar 7, 2010, at 23:41 , Johannes Berg wrote:

> Hmm, you're probably right, and I cannot test with my powerbook as I'm
> about to leave on travel and it's at my parents.

Any other maintainers I should forward it to? Looks like straightforward small bug.

Sincerely,
Vadim Zaliva

--
"Hated by fools, and fools to hate, be this my motto and my fate"
(Jonathan Swift)







[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3267 bytes --]

Re: [PATCH 1/1] integer overflow issue in 'appletouch' driver

[Dmitry Torokhov]

On Tue, Mar 09, 2010 at 11:43:25AM -0800, Vadim Zaliva wrote:
> On Mar 7, 2010, at 23:41 , Johannes Berg wrote:
> 
> > Hmm, you're probably right, and I cannot test with my powerbook as I'm
> > about to leave on travel and it's at my parents.
> 
> Any other maintainers I should forward it to? Looks like straightforward small bug.
> 

Just need your Signed-off-by please.

-- 
Dmitry

Re: [PATCH 1/1] integer overflow issue in 'appletouch' driver

[Serge Belyshev]

Hi!

Since 2.6.34 the touchpad on my feb 2005 powerbook (mod. A1106) has
stopped working. I've identified the following guilty patch:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=04b4b88cca0ebe3813b4b6f014fb6a0db380b137

> ... Testing on one of such PowerBooks I have
> noticed that touchpad always generates positive values, but some of
> them are greater that 127, and thus, when cast to 'signed char' being
> interpreted as a negative.

My device is 05ac:020e "fountain", actually generates *signed* values,
thus the patch completely breaks it.

Re: [PATCH 1/1] integer overflow issue in 'appletouch' driver

[Johannes Berg]

On Sat, 2010-08-07 at 14:00 +0000, Serge Belyshev wrote:
> Hi!
> 
> Since 2.6.34 the touchpad on my feb 2005 powerbook (mod. A1106) has
> stopped working. I've identified the following guilty patch:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=04b4b88cca0ebe3813b4b6f014fb6a0db380b137
> 
> > ... Testing on one of such PowerBooks I have
> > noticed that touchpad always generates positive values, but some of
> > them are greater that 127, and thus, when cast to 'signed char' being
> > interpreted as a negative.
> 
> My device is 05ac:020e "fountain", actually generates *signed* values,
> thus the patch completely breaks it.

I think Ben might have the same issue. I guess we need per-touchpad
functions to read the data.

johannes

Re: [PATCH 1/1] integer overflow issue in 'appletouch' driver

[Benjamin Herrenschmidt]

> > My device is 05ac:020e "fountain", actually generates *signed* values,
> > thus the patch completely breaks it.
> 
> I think Ben might have the same issue. I guess we need per-touchpad
> functions to read the data.

Yup, I confirm, same deal. This patch completely breaks it on my
powerbook (same USB ID)

Should we revert the commit for now ?

Cheers,
Ben.

Re: [PATCH 1/1] integer overflow issue in 'appletouch' driver

[Vadim Zaliva]

I have tested the patch on device I had       
I will have an access to it to retest later thus week. We can wait for my test to try to identify how my PowerBook is different from yours. Or you can roll the patch back. 

--
Sent from my iPhone

On Aug 8, 2010, at 17:20, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
>>> My device is 05ac:020e "fountain", actually generates *signed* values,
>>> thus the patch completely breaks it.
>> 
>> I think Ben might have the same issue. I guess we need per-touchpad
>> functions to read the data.
> 
> Yup, I confirm, same deal. This patch completely breaks it on my
> powerbook (same USB ID)
> 
> Should we revert the commit for now ?
> 
> Cheers,
> Ben.
> 

Re: [PATCH 1/1] integer overflow issue in 'appletouch' driver

[Benjamin Herrenschmidt]

On Sun, 2010-08-08 at 18:24 -0700, Vadim Zaliva wrote:
> I have tested the patch on device I had       
> I will have an access to it to retest later thus week. We can wait for my
> test to try to identify how my PowerBook is different from yours. Or you
> can roll the patch back.

I'd rather roll the patch back for now. I'll send the revert as part of
the next powerpc update. We'll sort things out then.

Cheers,
Ben.

> 
> --
> Sent from my iPhone
> 
> On Aug 8, 2010, at 17:20, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > 
> >>> My device is 05ac:020e "fountain", actually generates *signed* values,
> >>> thus the patch completely breaks it.
> >> 
> >> I think Ben might have the same issue. I guess we need per-touchpad
> >> functions to read the data.
> > 
> > Yup, I confirm, same deal. This patch completely breaks it on my
> > powerbook (same USB ID)
> > 
> > Should we revert the commit for now ?
> > 
> > Cheers,
> > Ben.
> > 

Re: [PATCH 1/1] integer overflow issue in 'appletouch' driver

[Dmitry Torokhov]

On Sunday, August 08, 2010 08:43:09 pm Benjamin Herrenschmidt wrote:
> On Sun, 2010-08-08 at 18:24 -0700, Vadim Zaliva wrote:
> > I have tested the patch on device I had
> > I will have an access to it to retest later thus week. We can wait for my
> > test to try to identify how my PowerBook is different from yours. Or you
> > can roll the patch back.
> 
> I'd rather roll the patch back for now. I'll send the revert as part of
> the next powerpc update. We'll sort things out then.

I'll be sending pull request to Linus this evening and should take care of
this as well. I guess we need to pull it out of stable as well since Vadim's
thouchpad was usable even with older code but some touchpads are completely
broken now.

We can re-instate the fix later when we make sure it works fr everyone.

Thanks.

-- 
Dmitry

Re: [PATCH 1/1] integer overflow issue in 'appletouch' driver

[Benjamin Herrenschmidt]

On Mon, 2010-08-09 at 09:33 -0700, Dmitry Torokhov wrote:
> 
> > I'd rather roll the patch back for now. I'll send the revert as part of
> > the next powerpc update. We'll sort things out then.
> 
> I'll be sending pull request to Linus this evening and should take care of
> this as well. I guess we need to pull it out of stable as well since Vadim's
> thouchpad was usable even with older code but some touchpads are completely
> broken now.

Right.

> We can re-instate the fix later when we make sure it works fr everyone.

Yup. BTW. The pull request I sent to Linus yesterday has a revert for
that. Can you take care of stable ?

Thanks !

Cheers,
Ben.

Re: [PATCH 1/1] integer overflow issue in 'appletouch' driver

[Dmitry Torokhov]

On Tue, Jun 19, 2018 at 1:23 PM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Sun, 2010-08-08 at 18:24 -0700, Vadim Zaliva wrote:
> > I have tested the patch on device I had
> > I will have an access to it to retest later thus week. We can wait for my
> > test to try to identify how my PowerBook is different from yours. Or you
> > can roll the patch back.
>
> I'd rather roll the patch back for now. I'll send the revert as part of
> the next powerpc update. We'll sort things out then.
>

Hmm, I just got this blast form the past and looking by the headers it
might be coming form Mauro?

Received: from mchehab by bombadil.infradead.org with local (Exim
4.90_1 #2 (Red Hat Linux)) id 1fVN9i-0002G4-I9; Tue, 19 Jun 2018
20:23:10 +0000
Received: from vger.kernel.org ([209.132.180.67]) by
bombadil.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id
1OiJGy-0000U8-Lw; Mon, 09 Aug 2010 03:43:36 +0000

Thanks.

-- 
Dmitry

Re: [PATCH 1/1] integer overflow issue in 'appletouch' driver

[Vadim Zaliva]

[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]


> Just need your Signed-off-by please.


Here it is:

Signed-off-by: Vadim Zaliva <lord@crocodile.org>

(including original patch to put it in context. Sorry if it seems like over-quoting)

Sincerely,
Vadim Zaliva

On Mar 5, 2010, at 11:47 , Vadim Zaliva wrote:

> This small patch is fixing an integer overflow issue in 'appletouch' driver.
> 
> In particular, reading data from Geyser 2 touchpads used on post Oct
> 2005 Apple PowerBooks the driver was casting X and Y coordinates
> values to 'signed char'. Testing on one of such PowerBooks I have
> noticed that touchpad always generates positive values, but some of
> them are greater that 127, and thus, when cast to 'signed char' being
> interpreted as a negative.
> 
> Such bigger values have been observed infrequently, closer to the
> edges of a touchpad, so the problem was not very visible. 
> Nevertheless, the patch would potentially improve touchpad
> driver accuracy.
> 
> 
> diff -uNr linux-source-2.6.31.orig/drivers/input/mouse/appletouch.c linux-source-2.6.31/drivers/input/mouse/appletouch.c
> --- linux-source-2.6.31.orig/drivers/input/mouse/appletouch.c	2009-09-09 15:13:59.000000000 -0700
> +++ linux-source-2.6.31/drivers/input/mouse/appletouch.c	2010-03-05 11:05:11.921394055 -0800
> @@ -205,8 +205,8 @@
> 	bool			overflow_warned;
> 	int			x_old;		/* last reported x/y, */
> 	int			y_old;		/* used for smoothing */
> -	signed char		xy_cur[ATP_XSENSORS + ATP_YSENSORS];
> -	signed char		xy_old[ATP_XSENSORS + ATP_YSENSORS];
> +	u8		    xy_cur[ATP_XSENSORS + ATP_YSENSORS];
> +	u8		    xy_old[ATP_XSENSORS + ATP_YSENSORS];
> 	int			xy_acc[ATP_XSENSORS + ATP_YSENSORS];
> 	int			idlecount;	/* number of empty packets */
> 	struct work_struct	work;
> @@ -531,7 +531,7 @@
> 
> 	for (i = 0; i < ATP_XSENSORS + ATP_YSENSORS; i++) {
> 		/* accumulate the change */
> -		signed char change = dev->xy_old[i] - dev->xy_cur[i];
> +		int change = dev->xy_old[i] - dev->xy_cur[i];
> 		dev->xy_acc[i] -= change;
> 
> 		/* prevent down drifting */
> 
> 


--
"Hated by fools, and fools to hate, be this my motto and my fate"
(Jonathan Swift)







[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3267 bytes --]