mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Qing Wang <wangqing7171@gmail.com>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <nick.desaulniers+lkml@gmail.com>,
	Bill Wendling <morbo@google.com>,
	Justin Stitt <justinstitt@google.com>,
	Thomas Gleixner <tglx@kernel.org>, Ingo Molnar <mingo@kernel.org>,
	Dmitry Vyukov <dvyukov@google.com>,
	Mark Rutland <mark.rutland@arm.com>,
	linux-kernel@vger.kernel.org, llvm@lists.linux.dev,
	syzbot+185a631927096f9da2fc@syzkaller.appspotmail.com
Subject: Re: [PATCH] rseq: fix using an uninitialized stack variable in rseq_exit_user_update
Date: Mon, 1 Jun 2026 16:39:34 +0200	[thread overview]
Message-ID: <20260601143934.GT3493090@noisy.programming.kicks-ass.net> (raw)
In-Reply-To: <5f8457e2-ffc5-4a0f-997d-a0f11641979c@efficios.com>

On Mon, Jun 01, 2026 at 09:59:35AM -0400, Mathieu Desnoyers wrote:
> On 2026-05-31 22:13, Qing Wang wrote:
> > There is an bug which is an uninitialized stack variable use in
> > `rseq_exit_user_update()` reported by syzbot:
> > 
> > BUG: KMSAN: kernel-infoleak in rseq_set_ids_get_csaddr include/linux/rseq_entry.h:502 [inline]
> > 
> > The local variable:
> > ```c
> > 	struct rseq_ids ids = {
> > 		.cpu_id	 = task_cpu(t),
> > 		.mm_cid	 = task_mm_cid(t),
> > 		.node_id = cpu_to_node(ids.cpu_id),
> > 	};
> > ```
> > According to the C standard, the evaluation order of expressions in an
> > initializer list is indeterminately sequenced. The compiler (Clang, in this
> > KMSAN build) evaluates `cpu_to_node(ids.cpu_id)` *before* `ids.cpu_id` is
> > initialized with `task_cpu(t)`.

W.T.F. would they (C committee) do that :-(, and since this is
apparently a known thing, why aren't the compilers issuing WARNs about
this pattern?

Clearly any non address-of self reference in a struct initializer is
buggered.

And ideally it would actually observe the data-dependency and respect
it, irrespective of the lunacy of the C standard.

> > This is fixed by moving the assignment of ids.node_id outside the structure
> > initialization.
> > 
> > Reported-by: syzbot+185a631927096f9da2fc@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=185a631927096f9da2fc
> > Fixes: 82f572449cfe ("rseq: Implement read only ABI enforcement for optimized RSEQ V2 mode")
> > Signed-off-by: Qing Wang <wangqing7171@gmail.com>
> > ---
> >   include/linux/rseq_entry.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/rseq_entry.h b/include/linux/rseq_entry.h
> > index 63bc72086e75..e05f4c18f39e 100644
> > --- a/include/linux/rseq_entry.h
> > +++ b/include/linux/rseq_entry.h
> > @@ -638,8 +638,8 @@ static __always_inline bool rseq_exit_user_update(struct pt_regs *regs, struct t
> >   	struct rseq_ids ids = {
> >   		.cpu_id	 = task_cpu(t),
> >   		.mm_cid	 = task_mm_cid(t),
> > -		.node_id = cpu_to_node(ids.cpu_id),
> >   	};
> > +	ids.node_id = cpu_to_node(ids.cpu_id);
> 
> I find the fix odd. How about:
> 
> int cpu = task_cpu(t);
> struct rseq_ids ids = {
> 	.cpu_id = cpu,
> 	.mm_cid = task_mm_cid(t),
> 	.node_id = cpu_to_node(cpu),
> };
> 
> instead ?

Yes, much saner.

  reply	other threads:[~2026-06-01 14:39 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-01  2:13 Qing Wang
2026-06-01 13:59 ` Mathieu Desnoyers
2026-06-01 14:39   ` Peter Zijlstra [this message]
2026-06-02  3:08     ` [PATCH v2] " Qing Wang
2026-06-02 10:08       ` Mark Rutland
2026-06-02 10:42       ` Peter Zijlstra
2026-06-19 12:45         ` Tetsuo Handa
2026-06-19 19:32           ` Thomas Gleixner
2026-06-19 22:34             ` Tetsuo Handa
2026-06-20 19:51               ` Thomas Gleixner
2026-06-25  9:18                 ` Alexander Potapenko
2026-06-02 10:45       ` [tip: sched/urgent] rseq: Fix using an uninitialized stack variable in rseq_exit_user_update() tip-bot2 for Qing Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260601143934.GT3493090@noisy.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=dvyukov@google.com \
    --cc=justinstitt@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=llvm@lists.linux.dev \
    --cc=mark.rutland@arm.com \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mingo@kernel.org \
    --cc=morbo@google.com \
    --cc=nathan@kernel.org \
    --cc=nick.desaulniers+lkml@gmail.com \
    --cc=syzbot+185a631927096f9da2fc@syzkaller.appspotmail.com \
    --cc=tglx@kernel.org \
    --cc=wangqing7171@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome