* [PATCH] Input: psmouse: check the result of PSMOUSE_CMD_GET* commands
@ 2021-11-29 14:38 Alexander Potapenko
2021-12-11 2:54 ` Dmitry Torokhov
0 siblings, 1 reply; 4+ messages in thread
From: Alexander Potapenko @ 2021-11-29 14:38 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: dvyukov, elver, linux-input, Alexander Potapenko
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 (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;
--
2.34.0.rc2.393.gf8c9666880-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] Input: psmouse: check the result of PSMOUSE_CMD_GET* commands
2021-11-29 14:38 [PATCH] Input: psmouse: check the result of PSMOUSE_CMD_GET* commands Alexander Potapenko
@ 2021-12-11 2:54 ` Dmitry Torokhov
2021-12-13 11:26 ` Alexander Potapenko
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Torokhov @ 2021-12-11 2:54 UTC (permalink / raw)
To: Alexander Potapenko; +Cc: dvyukov, elver, linux-input
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.
--
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Input: psmouse: check the result of PSMOUSE_CMD_GET* commands
2021-12-11 2:54 ` Dmitry Torokhov
@ 2021-12-13 11:26 ` Alexander Potapenko
2021-12-13 11:32 ` Alexander Potapenko
0 siblings, 1 reply; 4+ messages in thread
From: Alexander Potapenko @ 2021-12-13 11:26 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dvyukov, elver, linux-input
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
--
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Input: psmouse: check the result of PSMOUSE_CMD_GET* commands
2021-12-13 11:26 ` Alexander Potapenko
@ 2021-12-13 11:32 ` Alexander Potapenko
0 siblings, 0 replies; 4+ messages in thread
From: Alexander Potapenko @ 2021-12-13 11:32 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dvyukov, elver, linux-input
> 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
--
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-12-13 11:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-11-29 14:38 [PATCH] Input: psmouse: check the result of PSMOUSE_CMD_GET* commands Alexander Potapenko
2021-12-11 2:54 ` Dmitry Torokhov
2021-12-13 11:26 ` Alexander Potapenko
2021-12-13 11:32 ` Alexander Potapenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).