linux-hardening - Re: [tip: perf/core] perf/x86: Annotate struct bts_buffer with __counted

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: <Z8gvYIYXMHRC-btB@gmail.com>
Date: Wed, 5 Mar 2025 12:02:56 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Thorsten Blum <thorsten.blum@...ux.dev>
Cc: linux-kernel@...r.kernel.org, linux-tip-commits@...r.kernel.org,
	Peter Zijlstra <peterz@...radead.org>, x86@...nel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [tip: perf/core] perf/x86: Annotate struct bts_buffer with
 __counted_by()


* Thorsten Blum <thorsten.blum@...ux.dev> wrote:

> On 5. Mar 2025, at 10:18, Ingo Molnar wrote:
> > Actually, on a second thought:
> > 
> >> - buf = kzalloc_node(offsetof(struct bts_buffer, buf[nbuf]), GFP_KERNEL, node);
> >> + buf = kzalloc_node(struct_size(buf, buf, nbuf), GFP_KERNEL, node);
> > 
> > Firstly, in what world is 'buf, buf' more readable? One is a member of 
> > a structure, the other is the name of the structure - and they match, 
> > which shows that this function's naming conventions are a mess.
> > 
> > Which should be fixed first ...
> 
> Yes, I noticed this too, but since buf->buf[] is used all over the place
> (also in other functions), I didn't rename it in this patch.
> 
> We could just keep offsetof(struct bts_buffer, buf[nbuf]), or use
> struct_size_t(struct bts_buffer, buf, nbuf) and still benefit from
> additional compile-time checks, or rename the local variable to struct
> bts_buffer *bts and use struct_size(bts, buf, nbuf), for example. Any
> preferences or other ideas?

To clean up this code before changing it, so that the changes become 
obvious to review.

Please also split out the annotation for instrumentation, it's separate 
from any struct_size() changes, right?

> > I'm also not sure the code is correct ...
> 
> Which part of it?

The size calculation. On a second reading I *think* it's correct, but 
it's unnecessarily confusing due to the buf<->buf aliasing.

So in a cleaned up version of the code:

  - If we name 'struct bts_buffer' objects 'bb'
  - and bb:buf[] is the var-array
  - and we rename 'nbuf' to 'nr_buf' (the number of bb:buf[] elements)

then the code right now does:

        bb = kzalloc_node(offsetof(struct bts_buffer, bb[nr_buf]), GFP_KERNEL, node);

... which looks correct.

Thanks,

	Ingo