[lvc-project] [PATCH] media: rc: Fix use-after-free when racing vfd_write() with disconnect

[Fedor Pchelkin]

On Tue, 22. Jul 19:24, Larshin Sergey wrote:
> `imon_disconnect()` races with `vfd_write()`, `imon_disconnect()`

Наименования функций можно писать без кавычек. То что упоминается
функция, понятно по наличию скобок в конце. Количество `` можно
понизить.

> may complete the pending transfer and free `ictx`
> while `vfd_write()` is blocked in `wait_for_completion_interruptible()`.
> When `vfd_write()` wakes up, it accesses freed `ictx`,
> causing a use-after-free.
> 
> Thread 1 vfd_write()                       Thread 2 imon_disconnect()
> ...
> mutex_lock_interruptible(&ictx->lock)
> ...
> while
>   send_packet(ictx)
>     ictx->tx.busy = true // UAF
>     usb_submit_urb()
>     wait_for_completion_interruptible()
>     ...
>                                           ...
>                                           if (ictx->tx.busy)
>                                             usb_kill_urb(ictx->tx_urb)
>                                             complete(&ictx->tx.finished)
> 
>                                           ...
>                                           if (refcount_dec_and_test
>                                                          (&ictx->users))
>                                             free_imon_context(ictx)
> 
>     ictx->tx.busy = false // UAF

В https://syzkaller.appspot.com/bug?extid=f1a69784f6efe748c3bf UAF
происходит к struct usb_device.

Пока что из описания не ясно, в чём заключается проблема.

>     retval = ictx->tx.status
> 
> Set and read `ictx->disconnected` atomically.

Каким образом атомарность операций позволит что-то устранить при
наблюдаемой гонке?

> Acquire `ictx->lock` in `imon_disconnect()` after setting
> the flag to synchronize with writers holding the lock.
> This ensures writers exit safely before `disconnect()`
> proceeds with cleanup.

По регламентам работ требуется добавлять строчку

Found by Linux Verification Center (linuxtesting.org) with <tool>.

либо

Found by <organization> on behalf of Linux Verification Center (linuxtesting.org)
with <tool>.

https://portal.linuxtesting.ru/How-to-send-patches-to-kernel.html#Порядок-тегов

> 
> Reported-by: syzbot+f1a69784f6efe748c3bf at syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=f1a69784f6efe748c3bf
> Signed-off-by: Larshin Sergey <Sergey.Larshin at kaspersky.com>

Для патчей, устраняющих ошибки в ядре, желателен тег Fixes.
https://portal.linuxtesting.ru/How-to-send-patches-to-kernel.html#Fixes

> @@ -2499,7 +2507,11 @@ static void imon_disconnect(struct usb_interface *interface)
>  	int ifnum;
>  
>  	ictx = usb_get_intfdata(interface);
> -	ictx->disconnected = true;
> +	/* Ensure disconnect is visible to readers */
> +	smp_store_release(&ictx->disconnected, true);
> +	mutex_lock(&ictx->lock);
> +	mutex_unlock(&ictx->lock);

Ну это совсем уж dirty hack какой-то :)

Честно говоря, идея патча с использованием memory-ordering рутин
smp_load_acquire/smp_store_release для устранения гонки в драйвере не
понятна, т.к. параллельный поток, если он действительно параллельный,
может взять и изменить значение переменной сразу после её проверки

	/* Ensure we see device connect before proceeding */
	if (smp_load_acquire(&ictx->disconnected))
		return -ENODEV;

и полезность такой проверки становится равной нулю. А если секции чтения
и записи в ictx->disconnected обвешаны какими-нибудь локами, то зачем
нужен дополнительный smp_load_acquire()?

Возможно на что-то натолкнёт история этого драйвера, похожие гонки там уже
когда-то исправляли или пытались исправлять:

$ git log drivers/media/rc/imon.c

commit 24147897507cd3a7d63745d1518a638bf4132238
Author: Ricardo Ribalda <ribalda at chromium.org>
Date:   Mon May 6 21:10:27 2024 +0000

    media: imon: Fix race getting ictx->lock


commit 813ceef062b53d68f296aa3cb944b21a091fabdb
Author: Gautam Menghani <gautammenghani201 at gmail.com>
Date:   Wed Oct 19 06:02:14 2022 +0100

    media: imon: fix a race condition in send_packet()


commit db264d4c66c0fe007b5d19fd007707cd0697603d
Author: Tetsuo Handa <penguin-kernel at I-love.SAKURA.ne.jp>
Date:   Mon May 2 05:49:04 2022 +0200

    media: imon: reorganize serialization


> +
>  	dev = ictx->dev;
>  	ifnum = interface->cur_altsetting->desc.bInterfaceNumber;
>  
> @@ -2513,7 +2525,7 @@ static void imon_disconnect(struct usb_interface *interface)
>  	usb_set_intfdata(interface, NULL);
>  
>  	/* Abort ongoing write */
> -	if (ictx->tx.busy) {
> +	if (READ_ONCE(ictx->tx.busy)) {
>  		usb_kill_urb(ictx->tx_urb);
>  		complete(&ictx->tx.finished);
>  	}
> -- 
> 2.39.5