Input: psmouse: check the result of PSMOUSE_CMD_GET* commands

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

Hi Alexander,

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

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

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

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.

quoted

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