HID: roccat: Fix Use-After-Free in roccat_read

roccat_report_event() is responsible for registering
roccat-related reports in struct roccat_device.

int roccat_report_event(int minor, u8 const *data)
{
	struct roccat_device *device;
	struct roccat_reader *reader;
	struct roccat_report *report;
	uint8_t *new_value;

	device = devices[minor];

	new_value = kmemdup(data, device->report_size, GFP_ATOMIC);
	if (!new_value)
		return -ENOMEM;

	report = &device->cbuf[device->cbuf_end];

	/* passing NULL is safe */
	kfree(report->value);
	...

The registered report is stored in the struct roccat_device member
"struct roccat_report cbuf[ROCCAT_CBUF_SIZE];".
If more reports are received than the "ROCCAT_CBUF_SIZE" value,
kfree() the saved report from cbuf[0] and allocates a new reprot.
Since there is no lock when this kfree() is performed,
kfree() can be performed even while reading the saved report.

static ssize_t roccat_read(struct file *file, char __user *buffer,
		size_t count, loff_t *ppos)
{
	struct roccat_reader *reader = file->private_data;
	struct roccat_device *device = reader->device;
	struct roccat_report *report;
	ssize_t retval = 0, len;
	DECLARE_WAITQUEUE(wait, current);

	mutex_lock(&device->cbuf_lock);

	...

	report = &device->cbuf[reader->cbuf_start];
	/*
	 * If report is larger than requested amount of data, rest of report
	 * is lost!
	 */
	len = device->report_size > count ? count : device->report_size;

	if (copy_to_user(buffer, report->value, len)) {
		retval = -EFAULT;
		goto exit_unlock;
	}
	...

The roccat_read() function receives the device->cbuf report and
delivers it to the user through copy_to_user().
If the N+ROCCAT_CBUF_SIZE th report is received while copying of
the Nth report->value is in progress, the pointer that copy_to_user()
is working on is kfree()ed and UAF read may occur. (race condition)

Since the device node of this driver does not set separate permissions,
this is not a security vulnerability, but because it is used for
requesting screen display of profile or dpi settings,
a user using the roccat device can apply udev to this device node or
There is a possibility to use it by giving.

Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 drivers/hid/hid-roccat.c | 2 ++
 1 file changed, 2 insertions(+)

roccat_report_event() is responsible for registering
roccat-related reports in struct roccat_device.

int roccat_report_event(int minor, u8 const *data)
{
	struct roccat_device *device;
	struct roccat_reader *reader;
	struct roccat_report *report;
	uint8_t *new_value;

	device = devices[minor];

	new_value = kmemdup(data, device->report_size, GFP_ATOMIC);
	if (!new_value)
		return -ENOMEM;

	report = &device->cbuf[device->cbuf_end];

	/* passing NULL is safe */
	kfree(report->value);
	...

The registered report is stored in the struct roccat_device member
"struct roccat_report cbuf[ROCCAT_CBUF_SIZE];".
If more reports are received than the "ROCCAT_CBUF_SIZE" value,
kfree() the saved report from cbuf[0] and allocates a new reprot.
Since there is no lock when this kfree() is performed,
kfree() can be performed even while reading the saved report.

static ssize_t roccat_read(struct file *file, char __user *buffer,
		size_t count, loff_t *ppos)
{
	struct roccat_reader *reader = file->private_data;
	struct roccat_device *device = reader->device;
	struct roccat_report *report;
	ssize_t retval = 0, len;
	DECLARE_WAITQUEUE(wait, current);

	mutex_lock(&device->cbuf_lock);

	...

	report = &device->cbuf[reader->cbuf_start];
	/*
	 * If report is larger than requested amount of data, rest of report
	 * is lost!
	 */
	len = device->report_size > count ? count : device->report_size;

	if (copy_to_user(buffer, report->value, len)) {
		retval = -EFAULT;
		goto exit_unlock;
	}
	...

The roccat_read() function receives the device->cbuf report and
delivers it to the user through copy_to_user().
If the N+ROCCAT_CBUF_SIZE th report is received while copying of
the Nth report->value is in progress, the pointer that copy_to_user()
is working on is kfree()ed and UAF read may occur. (race condition)

Since the device node of this driver does not set separate permissions,
this is not a security vulnerability, but because it is used for
requesting screen display of profile or dpi settings,
a user using the roccat device can apply udev to this device node or
There is a possibility to use it by giving.

Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 drivers/hid/hid-roccat.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 26373b82fe81..6da80e442fdd 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -257,6 +257,8 @@ int roccat_report_event(int minor, u8 const *data)
 	if (!new_value)
 		return -ENOMEM;

+	mutex_lock(&device->cbuf_lock);
+
 	report = &device->cbuf[device->cbuf_end];

 	/* passing NULL is safe */
@@ -276,6 +278,8 @@ int roccat_report_event(int minor, u8 const *data)
 			reader->cbuf_start = (reader->cbuf_start + 1) % ROCCAT_CBUF_SIZE;
 	}

+	mutex_unlock(&device->cbuf_lock);
+
 	wake_up_interruptible(&device->wait);
 	return 0;
 }
--
2.25.1


On Sun, Jun 26, 2022 at 02:54:47PM +0200, Silvan Jegen wrote:
> Usually for resends one would add something like "RESEND" to the patch
> according to
> 
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Thank you for telling me.
Next time I will attach the RESEND tag.

> Wouldn't we have to protect this assignment with a lock as well? Otherwise
> the copy_to_user may end up with the pointer changing mid-copy which it
> may or may not be able to deal with.
> 
> >  	device->cbuf_end = (device->cbuf_end + 1) % ROCCAT_CBUF_SIZE;

Because device->cbuf_lock is used throughout the roccat_read() function that calls copy_to_user(), 
modifying the value of a "report" member that is already used as copy_to_user() does not cause UAF. 
(Modifying report->value or device->cbuf_end does not affect copy_to_user().)

However, it seems cleaner and safer to include the mutex in all references to 
report, device, and reader in roccat_report_event().

So it seems better to modify the mutex position as above.

Regards,
Hyunwoo Kim.

On Sun, 26 Jun 2022, Hyunwoo Kim wrote:

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 26373b82fe81..abe23ccd48e8 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
> @@ -260,7 +260,9 @@ int roccat_report_event(int minor, u8 const *data)
>  	report = &device->cbuf[device->cbuf_end];
>  
>  	/* passing NULL is safe */
> +	mutex_lock(&device->cbuf_lock);
>  	kfree(report->value);
> +	mutex_unlock(&device->cbuf_lock);
>  
>  	report->value = new_value;
>  	device->cbuf_end = (device->cbuf_end + 1) % ROCCAT_CBUF_SIZE;

Don't we actually need the mutex for much longer period during 
roccat_report_event()? At minimum it's also manipulating cbuf_end.

Thanks,

On Thu, Jul 21, 2022 at 12:00:05PM +0200, Jiri Kosina wrote:
> Don't we actually need the mutex for much longer period during 
> roccat_report_event()? At minimum it's also manipulating cbuf_end.

I modified the mutex to protect most of the roccat_report_event() 
in the second patch above sent in reply.
Looking again, I submitted the second patch with some confusion. Sorry.

Regards,
Hyunwoo Kim.

Dear all,

This drivers/hid/hid-roccat.c code doesn't seem to have been patched for a long time.
I'd appreciate it if you could tell me how to make the patch I submitted above take effect.

Regards,
Hyunwoo Kim.

Hi

Hyunwoo Kim <imv4bel@gmail.com> wrote:
> This drivers/hid/hid-roccat.c code doesn't seem to have been patched
> for a long time.  I'd appreciate it if you could tell me how to make
> the patch I submitted above take effect.

My suggestion would be to resubmit the second version of your patch. You
admitted that your second version wasn't that easy to parse because you
sent it in a reply to another mail (quoting your mail from the 21st of
July below).

> On  Thu, 21 Jul 2022 04:01:16 -0700, Hyunwoo Kim wrote:
> > On Thu, Jul 21, 2022 at 12:00:05PM +0200, Jiri Kosina wrote:
> > Don't we actually need the mutex for much longer period during
> > roccat_report_event()? At minimum it's also manipulating cbuf_end.
> 
> I modified the mutex to protect most of the roccat_report_event()
> in the second patch above sent in reply.
> Looking again, I submitted the second patch with some confusion. Sorry.

Maybe resending the second version (with the appropriate "[PATCH v2]
HID: ..."-subject) would motivate people to have a second look (though
there is never a guarantee for that as both reviewers and maintainers
seem to be in short supply) ...

Cheers,
Silvan

On Sun, Sep 04, 2022 at 09:16:21PM +0200, Silvan Jegen wrote:
> Maybe resending the second version (with the appropriate "[PATCH v2]
> HID: ..."-subject) would motivate people to have a second look (though
> there is never a guarantee for that as both reviewers and maintainers
> seem to be in short supply) ...

Thank you for telling me.
I will resubmit the v2 patch.

Regards,
Hyunwoo Kim.