lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190125130113.GA18589@redhat.com>
Date:   Fri, 25 Jan 2019 14:01:14 +0100
From:   Oleg Nesterov <oleg@...hat.com>
To:     Vladis Dronov <vdronov@...hat.com>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH] HID: debug: fix the ring buffer implementation

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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ