linux-kernel - Re: [PATCH] ring-buffer: fix array bounds checking

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: <20241218120009.47b76bba@gandalf.local.home>
Date: Wed, 18 Dec 2024 12:00:09 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Jeongjun Park <aha310510@...il.com>
Cc: Vincent Donnefort <vdonnefort@...gle.com>, mhiramat@...nel.org,
 mathieu.desnoyers@...icios.com, david@...hat.com,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH] ring-buffer: fix array bounds checking

On Tue, 17 Dec 2024 22:42:53 +0900
Jeongjun Park <aha310510@...il.com> wrote:

> > Could you share that reproducer? Or at least the steps. As this situation should
> > never happen a, follow-up fix will be necessary.  
> 
> [1] When tested with a reproducer, pgoff was 8, subbuf_order was 0, and
> subbuf_pages was 1. However, nr_subbufs was 3, so oob-read or uaf occurred.
> 
> [1] : https://syzkaller.appspot.com/text?tag=ReproC&x=14514730580000

This was fixed by Edwards patch.

> Okay. In that case, I will just remove the variable declaration related patches
> and send you the v2 patch right away.
> 

I'm not sure this is needed nor is it a bug.

	while (p < nr_pages) {
		struct page *page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
		int off = 0;

		if (WARN_ON_ONCE(s >= nr_subbufs)) {
			err = -EINVAL;
			goto out;
		}

The WARN_ON_ONCE() suggests that this should never happen. And I believe it shouldn't.

I'm fine if you want to make the change to:

	while (p < nr_pages) {
		struct page *page;
		int off = 0;

		if (WARN_ON_ONCE(s >= nr_subbufs)) {
			err = -EINVAL;
			goto out;
		}

		page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);

But it's not a bug fix. It's simply a cleanup that can wait till the next
merge window.

-- Steve