Re: [PATCH] HID: hid-steam: fix uninit-value access in steam_get_serial

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing payload length validation in `steam_get_serial()` allows a short report to produce an empty serial number, leading to list corruption (Use-After-Free) during device unregistration.
- [High] Probe error path leaves a freed object in the global `steam_devices` list, causing a Use-After-Free.
- [Medium] Incorrect size argument passed to `strscpy()` causes truncation of the last character of the device's serial number.
--

commit ecebdbfdb0a084297c080f3d3ad65a720d3b9ec6
Author: Pei Xiao <xiaopei01@kylinos.cn>

HID: hid-steam: fix uninit-value access in steam_get_serial

The reply buffer in steam_get_serial() is zero-initialized to prevent KMSAN
uninit-value warnings. These warnings occur when steam_recv_report()
returns a short read or an error.

Reported-by: syzbot+75f3f9bff8c510602d36@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/lkml/6a27d055.39669fcc.33b062.008d.GAE@google.com/
Fixes: c164d6abf384 ("HID: add driver for Valve Steam Controller")
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>

> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 197126d6e0810..f65ad1a21c018 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -445,7 +445,7 @@ static int steam_get_serial(struct steam_device *steam)
>  	 */
>  	int ret = 0;
>  	u8 cmd[] = {ID_GET_STRING_ATTRIBUTE, sizeof(steam->serial_no), ATTRIB_STR_UNIT_SERIAL};
> -	u8 reply[3 + STEAM_SERIAL_LEN + 1];
> +	u8 reply[3 + STEAM_SERIAL_LEN + 1] = { 0 };
>  
>  	mutex_lock(&steam->report_mutex);
>  	ret = steam_send_report(steam, cmd, sizeof(cmd));

[Severity: High]
This is a pre-existing issue, but does steam_get_serial() properly validate
the payload length before copying the serial number?

If a device returns a short report (e.g., 4 bytes), steam_recv_report()
returns 4. The buffer validation checks reply[0] through reply[2], and if
they match, execution reaches the copy:

drivers/hid/hid-steam.c:steam_get_serial() {
    ...
    strscpy(steam->serial_no, reply + 3, reply[1]);
    ...
}

If reply[1] is 1, strscpy() will copy 0 characters and write a null
terminator at steam->serial_no[0]. steam_get_serial() then returns 4
(success), bypassing the fallback assignment in steam_register().

Later, during device unregistration:

drivers/hid/hid-steam.c:steam_unregister() {
    if (steam->serial_no[0]) {
        ...
        list_del_init(&steam->list);
        ...
    }
}

Could this skip list removal because steam->serial_no[0] is null, leaving a
freed object on the global steam_devices list and causing a use-after-free?

[Severity: Medium]
This isn't a bug introduced by this patch, but does the strscpy() call inside
steam_get_serial() pass the correct size argument?

drivers/hid/hid-steam.c:steam_get_serial() {
    ...
    strscpy(steam->serial_no, reply + 3, reply[1]);
    ...
}

Since strscpy() reserves one byte for the null terminator, passing the exact
string length (reply[1]) as the size will cause it to copy only reply[1] - 1
characters. Will this truncate the last character of a valid serial number if
the device specifies its exact length in reply[1]?

[Severity: High]
This is a pre-existing issue, but does steam_register() properly clean up
the global steam_devices list on failure?

During registration, the device is added to the global list:

drivers/hid/hid-steam.c:steam_register() {
    ...
    mutex_lock(&steam_devices_lock);
    if (list_empty(&steam->list))
        list_add(&steam->list, &steam_devices);
    mutex_unlock(&steam_devices_lock);
    ...
    ret = steam_input_register(steam);
    if (ret != 0)
        goto steam_register_input_fail;
    ...
steam_register_input_fail:
    return ret;
}

If steam_input_register() fails, steam_register() returns an error without
removing the object from the steam_devices list.

In the probe function:

drivers/hid/hid-steam.c:steam_probe() {
    ...
    ret = steam_register(steam);
    if (ret) {
        hid_err(hdev,
            "%s:steam_register failed with error %d\n",
            __func__, ret);
        goto err_hw_close;
    }
    ...
}

If steam_register() fails, the driver core will free the devres-managed
steam_device struct. Could this leave a dangling pointer in the global
steam_devices list and lead to a use-after-free upon later list traversal?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0c5f5bcd3d01776615a97d7f23b5c1bff6a2acce.1781092734.git.xiaopei01@kylinos.cn?part=1