Re: [PATCH] Input: psmouse: check the result of PSMOUSE_CMD_GET* commands

On Sat, Dec 11, 2021 at 3:55 AM Dmitry Torokhov
[off-list ref] wrote:

Hi Alexander,

On Mon, Nov 29, 2021 at 03:38:45PM +0100, Alexander Potapenko wrote:

quoted

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