linux-kernel - Re: [PATCH v2] kcov, usb: Fix invalid context sleep in softirq path on PREEMPT

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] [day] [month] [year] [list]

Message-ID: <20250803084924.3785-1-hdanton@sina.com>
Date: Sun,  3 Aug 2025 16:49:22 +0800
From: Hillf Danton <hdanton@...a.com>
To: Yunseong Kim <ysk@...lloc.com>
Cc: Dmitry Vyukov <dvyukov@...gle.com>,
	Andrey Konovalov <andreyknvl@...il.com>,
	linux-kernel@...r.kernel.org,
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Uladzislau Rezki <urezki@...il.com>
Subject: Re: [PATCH v2] kcov, usb: Fix invalid context sleep in softirq path on PREEMPT_RT

On Sun, 3 Aug 2025 12:07:11 +0900 Yunseong Kim wrote:
> On 8/3/25 11:34 오전, Hillf Danton wrote:
> > On Sat,  2 Aug 2025 14:26:49 +0000 Yunseong Kim wrote:
> >> +	raw_spin_unlock(&kcov_remote_lock);
> >>  
> >>  	/* Can only happen when in_task(). */
> >>  	if (!area) {
> 
> 		/* 1. Interrupts are temporarily re-enabled here. */
> 
> >> -		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
> >> +		local_irq_restore(flags);
> 
> 		/* 2. vmalloc() is called safely in a non-atomic context. */
> 
> >>  		area = vmalloc(size * sizeof(unsigned long));
> > 
> > Given irq disabled for the duration of the coverage collection section [1],
> > vmalloc does not work here [2].
> 
> 
> Thank you for the detailed review and for pointing out this critical
> interaction. You are absolutely correct that vmalloc() cannot be called
> from an atomic context with interrupts disabled, as it is a sleeping function.
> 
> However, upon closer inspection of the kcov_remote_start() function's
> control flow, it appears the original author anticipated this issue and
> implemented a safeguard. The vmalloc() call is explicitly wrapped by
> local_irq_restore() and local_irq_save():
> 
> >>  		if (!area) {
> >>  			kcov_put(kcov);
> >>  			return;
> >>  		}
> 
> 		/* 3. Interrupts are disabled again to protect the rest of the function. */
> 
> >> -		local_lock_irqsave(&kcov_percpu_data.lock, flags);
> >> +		local_irq_save(flags);
> >>  	}
> 
> This sequence ensures that the vmalloc() call itself does not happen in an
> IRQ-disabled context.
> 
> My patch reverts the per-CPU locking back to the local_irq_save/restore
> primitives but preserves this essential bracketing around the vmalloc() call.
> Therefore, the sleeping function bug should not occur.
> 
What you missed is the local_irq_save in kcov_remote_start_usb_softirq().
See comment [1] for detail.

> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/kcov.h#n73
> > [2] Subject: [RFC 0/7] vmallloc and non-blocking GFPs
> > https://lore.kernel.org/all/20250704152537.55724-1-urezki@gmail.com/
> 
> Additionally, I have tested this implementation by running syzkaller
> for a full day, and no issues were reported.
> 
> Perhaps a comment could be added here (I can volunteer to do so) to
> improve readability where the control flow isn’t obvious to future developers.
> 
> Thanks,
> 
> Yunseong Kim
> 
>