| Message ID | 1424726243-115451-1-git-send-email-seth.forshee@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 23.02.2015 22:17, Seth Forshee wrote: > d1c7e29e8d27 (HID: i2c-hid: prevent buffer overflow in early IRQ) > changed hid_get_input() to read ihid->bufsize bytes, which can be > more than wMaxInputLength. This is the case with the Dell XPS 13 > 9343, and it is causing events to be missed. In some cases the > missed events are releases, which can cause the cursor to jump or > freeze, among other problems. Limit the number of bytes read to > min(wMaxInputLength, ihid->bufsize) to prevent such problems. Possibly this is not only relevant for Vivid... https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1425445 -Stefan > > Fixes: d1c7e29e8d27 "HID: i2c-hid: prevent buffer overflow in early IRQ" > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Signed-off-by: Jiri Kosina <jkosina@suse.cz> > (cherry picked from commit 6d00f37e49d95e640a3937a4a1ae07dbe92a10cb > git://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git) > --- > > This should come in from upstream stable eventually, but for selfish > reasons I'd like to get it in sooner. I'm targeting 3.19 since I'm not > anticipating any more 3.18 releases for vivid, but it should apply fine > to 3.18 as well. > > drivers/hid/i2c-hid/i2c-hid.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c > index d43e967..5e72fc2 100644 > --- a/drivers/hid/i2c-hid/i2c-hid.c > +++ b/drivers/hid/i2c-hid/i2c-hid.c > @@ -370,7 +370,10 @@ static int i2c_hid_hwreset(struct i2c_client *client) > static void i2c_hid_get_input(struct i2c_hid *ihid) > { > int ret, ret_size; > - int size = ihid->bufsize; > + int size = le16_to_cpu(ihid->hdesc.wMaxInputLength); > + > + if (size > ihid->bufsize) > + size = ihid->bufsize; > > ret = i2c_master_recv(ihid->client, ihid->inbuf, size); > if (ret != size) { >
On Mon, Feb 23, 2015 at 03:17:23PM -0600, Seth Forshee wrote: > d1c7e29e8d27 (HID: i2c-hid: prevent buffer overflow in early IRQ) > changed hid_get_input() to read ihid->bufsize bytes, which can be > more than wMaxInputLength. This is the case with the Dell XPS 13 > 9343, and it is causing events to be missed. In some cases the > missed events are releases, which can cause the cursor to jump or > freeze, among other problems. Limit the number of bytes read to > min(wMaxInputLength, ihid->bufsize) to prevent such problems. > > Fixes: d1c7e29e8d27 "HID: i2c-hid: prevent buffer overflow in early IRQ" > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Signed-off-by: Jiri Kosina <jkosina@suse.cz> > (cherry picked from commit 6d00f37e49d95e640a3937a4a1ae07dbe92a10cb > git://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git) > --- > > This should come in from upstream stable eventually, but for selfish > reasons I'd like to get it in sooner. I'm targeting 3.19 since I'm not > anticipating any more 3.18 releases for vivid, but it should apply fine > to 3.18 as well. > Unfortunately this commit has *not* been tagged for stable. Since we're having issues reported against utopic kernels as well (thanks for the heads-up Stephen), I'm queuing it for the 3.16 stable kernel. But it may be worth also SRU'ing it for utopic. Cheers, -- Luís > drivers/hid/i2c-hid/i2c-hid.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c > index d43e967..5e72fc2 100644 > --- a/drivers/hid/i2c-hid/i2c-hid.c > +++ b/drivers/hid/i2c-hid/i2c-hid.c > @@ -370,7 +370,10 @@ static int i2c_hid_hwreset(struct i2c_client *client) > static void i2c_hid_get_input(struct i2c_hid *ihid) > { > int ret, ret_size; > - int size = ihid->bufsize; > + int size = le16_to_cpu(ihid->hdesc.wMaxInputLength); > + > + if (size > ihid->bufsize) > + size = ihid->bufsize; > > ret = i2c_master_recv(ihid->client, ihid->inbuf, size); > if (ret != size) { > -- > 1.9.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Wed, Feb 25, 2015 at 11:08:02AM +0000, Luis Henriques wrote: > On Mon, Feb 23, 2015 at 03:17:23PM -0600, Seth Forshee wrote: > > d1c7e29e8d27 (HID: i2c-hid: prevent buffer overflow in early IRQ) > > changed hid_get_input() to read ihid->bufsize bytes, which can be > > more than wMaxInputLength. This is the case with the Dell XPS 13 > > 9343, and it is causing events to be missed. In some cases the > > missed events are releases, which can cause the cursor to jump or > > freeze, among other problems. Limit the number of bytes read to > > min(wMaxInputLength, ihid->bufsize) to prevent such problems. > > > > Fixes: d1c7e29e8d27 "HID: i2c-hid: prevent buffer overflow in early IRQ" > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > Signed-off-by: Jiri Kosina <jkosina@suse.cz> > > (cherry picked from commit 6d00f37e49d95e640a3937a4a1ae07dbe92a10cb > > git://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git) > > --- > > > > This should come in from upstream stable eventually, but for selfish > > reasons I'd like to get it in sooner. I'm targeting 3.19 since I'm not > > anticipating any more 3.18 releases for vivid, but it should apply fine > > to 3.18 as well. > > > > Unfortunately this commit has *not* been tagged for stable. Since > we're having issues reported against utopic kernels as well (thanks > for the heads-up Stephen), I'm queuing it for the 3.16 stable kernel. > But it may be worth also SRU'ing it for utopic. Oh, the wording in Documentation/SubmittingPatches lead me to believe that including the Fixes tag was sufficient get the stable kernels to pick it up. Apologies if I was mistaken. It does seem like it would make sense though for the stable tooling to trigger off those tags and see if the given sha1 is present in the stable tree, both directly and via the "commit <foo> upstream" messages. Seth
On Wed, Feb 25, 2015 at 08:02:57AM -0600, Seth Forshee wrote: > On Wed, Feb 25, 2015 at 11:08:02AM +0000, Luis Henriques wrote: > > On Mon, Feb 23, 2015 at 03:17:23PM -0600, Seth Forshee wrote: > > > d1c7e29e8d27 (HID: i2c-hid: prevent buffer overflow in early IRQ) > > > changed hid_get_input() to read ihid->bufsize bytes, which can be > > > more than wMaxInputLength. This is the case with the Dell XPS 13 > > > 9343, and it is causing events to be missed. In some cases the > > > missed events are releases, which can cause the cursor to jump or > > > freeze, among other problems. Limit the number of bytes read to > > > min(wMaxInputLength, ihid->bufsize) to prevent such problems. > > > > > > Fixes: d1c7e29e8d27 "HID: i2c-hid: prevent buffer overflow in early IRQ" > > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > > > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > Signed-off-by: Jiri Kosina <jkosina@suse.cz> > > > (cherry picked from commit 6d00f37e49d95e640a3937a4a1ae07dbe92a10cb > > > git://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git) > > > --- > > > > > > This should come in from upstream stable eventually, but for selfish > > > reasons I'd like to get it in sooner. I'm targeting 3.19 since I'm not > > > anticipating any more 3.18 releases for vivid, but it should apply fine > > > to 3.18 as well. > > > > > > > Unfortunately this commit has *not* been tagged for stable. Since > > we're having issues reported against utopic kernels as well (thanks > > for the heads-up Stephen), I'm queuing it for the 3.16 stable kernel. > > But it may be worth also SRU'ing it for utopic. > > Oh, the wording in Documentation/SubmittingPatches lead me to believe > that including the Fixes tag was sufficient get the stable kernels to > pick it up. Apologies if I was mistaken. > The authoritative document for stable patches is actually Documentation/stable_kernel_rules.txt and it doesn't actually refer to this 'Fixes' tag. Patches meant to be in stable kernels should have the 'Cc: stable@vger.kernel.org' tag. > It does seem like it would make sense though for the stable tooling to > trigger off those tags and see if the given sha1 is present in the > stable tree, both directly and via the "commit <foo> upstream" messages. Indeed, and I do have a local change to our tools to actually do this (I'll push it into kteam-tools *really* soon). The problem is that this will always require more manual intervention. A patch that contains a 'Fixes:' tag for a SHA1 that is present in a stable kernel doesn't necessarily meet the stable criteria. Of course all the other patches queued for stable need to be reviewed, but these will require special attention. Cheers, -- Luís
On Wed, Feb 25, 2015 at 02:34:59PM +0000, Luis Henriques wrote: > On Wed, Feb 25, 2015 at 08:02:57AM -0600, Seth Forshee wrote: > > On Wed, Feb 25, 2015 at 11:08:02AM +0000, Luis Henriques wrote: > > > On Mon, Feb 23, 2015 at 03:17:23PM -0600, Seth Forshee wrote: > > > > d1c7e29e8d27 (HID: i2c-hid: prevent buffer overflow in early IRQ) > > > > changed hid_get_input() to read ihid->bufsize bytes, which can be > > > > more than wMaxInputLength. This is the case with the Dell XPS 13 > > > > 9343, and it is causing events to be missed. In some cases the > > > > missed events are releases, which can cause the cursor to jump or > > > > freeze, among other problems. Limit the number of bytes read to > > > > min(wMaxInputLength, ihid->bufsize) to prevent such problems. > > > > > > > > Fixes: d1c7e29e8d27 "HID: i2c-hid: prevent buffer overflow in early IRQ" > > > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > > > > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > Signed-off-by: Jiri Kosina <jkosina@suse.cz> > > > > (cherry picked from commit 6d00f37e49d95e640a3937a4a1ae07dbe92a10cb > > > > git://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git) > > > > --- > > > > > > > > This should come in from upstream stable eventually, but for selfish > > > > reasons I'd like to get it in sooner. I'm targeting 3.19 since I'm not > > > > anticipating any more 3.18 releases for vivid, but it should apply fine > > > > to 3.18 as well. > > > > > > > > > > Unfortunately this commit has *not* been tagged for stable. Since > > > we're having issues reported against utopic kernels as well (thanks > > > for the heads-up Stephen), I'm queuing it for the 3.16 stable kernel. > > > But it may be worth also SRU'ing it for utopic. > > > > Oh, the wording in Documentation/SubmittingPatches lead me to believe > > that including the Fixes tag was sufficient get the stable kernels to > > pick it up. Apologies if I was mistaken. > > > > The authoritative document for stable patches is actually > Documentation/stable_kernel_rules.txt and it doesn't actually refer to > this 'Fixes' tag. Patches meant to be in stable kernels should have > the 'Cc: stable@vger.kernel.org' tag. > > > It does seem like it would make sense though for the stable tooling to > > trigger off those tags and see if the given sha1 is present in the > > stable tree, both directly and via the "commit <foo> upstream" messages. > > Indeed, and I do have a local change to our tools to actually do this > (I'll push it into kteam-tools *really* soon). The problem is that > this will always require more manual intervention. A patch that > contains a 'Fixes:' tag for a SHA1 that is present in a stable kernel > doesn't necessarily meet the stable criteria. Of course all the other > patches queued for stable need to be reviewed, but these will require > special attention. All right, I'll be sure to include the Cc stable in the future. Sorry again for the mix-up. Seth
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index d43e967..5e72fc2 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -370,7 +370,10 @@ static int i2c_hid_hwreset(struct i2c_client *client) static void i2c_hid_get_input(struct i2c_hid *ihid) { int ret, ret_size; - int size = ihid->bufsize; + int size = le16_to_cpu(ihid->hdesc.wMaxInputLength); + + if (size > ihid->bufsize) + size = ihid->bufsize; ret = i2c_master_recv(ihid->client, ihid->inbuf, size); if (ret != size) {