On 01/25, Vladis Dronov wrote:
>
> Ring buffer implementation in hid_debug_event() and hid_debug_events_read()
> is strange allowing lost or corrupted data. After commit 717adfdaf147
> ("HID: debug: check length before copy_to_user()") it is possible to enter
> an infinite loop in hid_debug_events_read() by providing 0 as count, this
> locks up a system. Fix this by rewriting the ring buffer implementation
> with kfifo and simplify the code.
>
> This fixes CVE-2019-3819.
To me this looks like a good cleanup even if we forget about bugfix. Cosmetic
nits, feel free to ignore...
> + if (kfifo_is_empty(&list->hid_debug_fifo)) {
> + add_wait_queue(&list->hdev->debug_wait, &wait);
> + set_current_state(TASK_INTERRUPTIBLE);
> +
> + while (kfifo_is_empty(&list->hid_debug_fifo)) {
> + if (file->f_flags & O_NONBLOCK) {
> + ret = -EAGAIN;
> + break;
> + }
> +
> + if (signal_pending(current)) {
> + ret = -ERESTARTSYS;
> + break;
> + }
> +
> + if (!list->hdev || !list->hdev->debug) {
> + ret = -EIO;
> + set_current_state(TASK_RUNNING);
> + goto out;
Can't resist... Yes, this is what the current code does. But you know that it looks
suspicious ;) if you add a comment the patch will be even better.
> + }
> +
> + /* allow O_NONBLOCK from other threads */
> + mutex_unlock(&list->read_mutex);
> + schedule();
> + mutex_lock(&list->read_mutex);
> + set_current_state(TASK_INTERRUPTIBLE);
> + }
> +
> + set_current_state(TASK_RUNNING);
you can use __set_current_state() here, mb() is not needed.
> + remove_wait_queue(&list->hdev->debug_wait, &wait);
> + }
> +
> + if (ret)
> + goto out;
> +
perhaps it make sense to move this check into the "if (kfifo_is_empty())" block.
> + if (kfifo_is_empty(&list->hid_debug_fifo))
> + goto out;
is kfifo_is_empty() == T really possible here?
Oleg.
