| Message ID | 20211129143845.1472453-1-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: psmouse: check the result of PSMOUSE_CMD_GET* commands | expand |
Hi Alexander, On Mon, Nov 29, 2021 at 03:38:45PM +0100, Alexander Potapenko wrote: > Execution of a PSMOUSE_CMD_GET* command may fail, leaving the output > buffer uninitialized. Make sure to check the return value of > ps2_command() and bail out before checking the buffer contents. > > The use of uninitialized data in genius_detect() was detected by KMSAN, > other places were fixed for the sake of uniformity. > > Signed-off-by: Alexander Potapenko <glider@google.com> > --- > drivers/input/mouse/psmouse-base.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c > index 0b4a3039f312f..a3305653ce891 100644 > --- a/drivers/input/mouse/psmouse-base.c > +++ b/drivers/input/mouse/psmouse-base.c > @@ -546,13 +546,16 @@ static int genius_detect(struct psmouse *psmouse, bool set_properties) > { > struct ps2dev *ps2dev = &psmouse->ps2dev; > u8 param[4]; > + int error; > > param[0] = 3; > ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES); > ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE11); > ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE11); > ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE11); > - ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO); > + error = ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO); > + if (error) > + return error; If we care about this I think we should care about errors from the rest of ps2_command()s. Otherwise we might have buffer initialized, but we are still checking potential garbage in it. Thanks.
On Sat, Dec 11, 2021 at 3:55 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > Hi Alexander, > > On Mon, Nov 29, 2021 at 03:38:45PM +0100, Alexander Potapenko wrote: > > Execution of a PSMOUSE_CMD_GET* command may fail, leaving the output > > buffer uninitialized. Make sure to check the return value of > > ps2_command() and bail out before checking the buffer contents. > > > > The use of uninitialized data in genius_detect() was detected by KMSAN, > > other places were fixed for the sake of uniformity. > > > > Signed-off-by: Alexander Potapenko <glider@google.com> > > --- > > drivers/input/mouse/psmouse-base.c | 21 ++++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c > > index 0b4a3039f312f..a3305653ce891 100644 > > --- a/drivers/input/mouse/psmouse-base.c > > +++ b/drivers/input/mouse/psmouse-base.c > > @@ -546,13 +546,16 @@ static int genius_detect(struct psmouse *psmouse, bool set_properties) > > { > > struct ps2dev *ps2dev = &psmouse->ps2dev; > > u8 param[4]; > > + int error; > > > > param[0] = 3; > > ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES); > > ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE11); > > ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE11); > > ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE11); > > - ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO); > > + error = ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO); > > + if (error) > > + return error; > > If we care about this I think we should care about errors from the rest > of ps2_command()s. Otherwise we might have buffer initialized, but we > are still checking potential garbage in it. Ok, I am going to add error checking to ps2_command()s that occur in the device detection code, because it's a sane assumption that every command in the device handshake should succeed. The only exception I see is the IntelliMouse 4.0 support code in im_explorer_detect (https://elixir.bootlin.com/linux/latest/source/drivers/input/mouse/psmouse-base.c#L628), where it is unclear whether these functions must succeed or not. I also won't be touching calls to ps2_command() in void functions which do not return errors anyway, and PSMOUSE_CMD_RESET_DIS commands, for which it is also unclear what to return. I think it makes sense to bail out if one of the ps2_command() preceding PSMOUSE_CMD_GET* returned an error, but am not sure The bugs caused by incorrect uses of PSMOUSE_CMD_GET are blocking KMSAN builds on syzbot, so we can immediately test the effect of the proposed change. If there were > Thanks. > > -- > Dmitry
> I think it makes sense to bail out if one of the ps2_command() > preceding PSMOUSE_CMD_GET* returned an error, but am not sure Sorry, please disregard this part. > The bugs caused by incorrect uses of PSMOUSE_CMD_GET are blocking > KMSAN builds on syzbot, so we can immediately test the effect of the > proposed change. This was meant to give some context, but I failed to finish my thought properly. Syzbot doesn't cover anything past mouse detection, so changing anything besides that couldn't be tested anyway. > If there were Disregard this as well. > > > Thanks. > > > > -- > > Dmitry > > > > -- > Alexander Potapenko > Software Engineer > > Google Germany GmbH > Erika-Mann-Straße, 33 > 80636 München > > Geschäftsführer: Paul Manicle, Halimah DeLaine Prado > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg
diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c index 0b4a3039f312f..a3305653ce891 100644 --- a/drivers/input/mouse/psmouse-base.c +++ b/drivers/input/mouse/psmouse-base.c @@ -546,13 +546,16 @@ static int genius_detect(struct psmouse *psmouse, bool set_properties) { struct ps2dev *ps2dev = &psmouse->ps2dev; u8 param[4]; + int error; param[0] = 3; ps2_command(ps2dev, param, PSMOUSE_CMD_SETRES); ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE11); ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE11); ps2_command(ps2dev, NULL, PSMOUSE_CMD_SETSCALE11); - ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO); + error = ps2_command(ps2dev, param, PSMOUSE_CMD_GETINFO); + if (error) + return error; if (param[0] != 0x00 || param[1] != 0x33 || param[2] != 0x55) return -ENODEV; @@ -578,6 +581,7 @@ static int intellimouse_detect(struct psmouse *psmouse, bool set_properties) { struct ps2dev *ps2dev = &psmouse->ps2dev; u8 param[2]; + int error; param[0] = 200; ps2_command(ps2dev, param, PSMOUSE_CMD_SETRATE); @@ -585,7 +589,9 @@ static int intellimouse_detect(struct psmouse *psmouse, bool set_properties) ps2_command(ps2dev, param, PSMOUSE_CMD_SETRATE); param[0] = 80; ps2_command(ps2dev, param, PSMOUSE_CMD_SETRATE); - ps2_command(ps2dev, param, PSMOUSE_CMD_GETID); + error = ps2_command(ps2dev, param, PSMOUSE_CMD_GETID); + if (error) + return error; if (param[0] != 3) return -ENODEV; @@ -611,6 +617,7 @@ static int im_explorer_detect(struct psmouse *psmouse, bool set_properties) { struct ps2dev *ps2dev = &psmouse->ps2dev; u8 param[2]; + int error; intellimouse_detect(psmouse, 0); @@ -620,7 +627,9 @@ static int im_explorer_detect(struct psmouse *psmouse, bool set_properties) ps2_command(ps2dev, param, PSMOUSE_CMD_SETRATE); param[0] = 80; ps2_command(ps2dev, param, PSMOUSE_CMD_SETRATE); - ps2_command(ps2dev, param, PSMOUSE_CMD_GETID); + error = ps2_command(ps2dev, param, PSMOUSE_CMD_GETID); + if (error) + return error; if (param[0] != 4) return -ENODEV; @@ -658,7 +667,7 @@ static int thinking_detect(struct psmouse *psmouse, bool set_properties) struct ps2dev *ps2dev = &psmouse->ps2dev; u8 param[2]; static const u8 seq[] = { 20, 60, 40, 20, 20, 60, 40, 20, 20 }; - int i; + int error, i; param[0] = 10; ps2_command(ps2dev, param, PSMOUSE_CMD_SETRATE); @@ -668,7 +677,9 @@ static int thinking_detect(struct psmouse *psmouse, bool set_properties) param[0] = seq[i]; ps2_command(ps2dev, param, PSMOUSE_CMD_SETRATE); } - ps2_command(ps2dev, param, PSMOUSE_CMD_GETID); + error = ps2_command(ps2dev, param, PSMOUSE_CMD_GETID); + if (error) + return error; if (param[0] != 2) return -ENODEV;
Execution of a PSMOUSE_CMD_GET* command may fail, leaving the output buffer uninitialized. Make sure to check the return value of ps2_command() and bail out before checking the buffer contents. The use of uninitialized data in genius_detect() was detected by KMSAN, other places were fixed for the sake of uniformity. Signed-off-by: Alexander Potapenko <glider@google.com> --- drivers/input/mouse/psmouse-base.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)