Hi 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 One question about the patch below (note that I am not very familiar with Linux Kernel driver code). Hyunwoo Kim <imv4bel@xxxxxxxxx> wrote: > 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@xxxxxxxxx> > --- > drivers/hid/hid-roccat.c | 2 ++ > 1 file changed, 2 insertions(+) > > 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; 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. Cheers, Silvan > device->cbuf_end = (device->cbuf_end + 1) % ROCCAT_CBUF_SIZE;
