linux-kernel - Re: [PATCH v10 1/7] lib/stackdepot: Fix first entry having a 0-handle

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 15 Feb 2024 16:25:54 -0800
From: Peter Collingbourne <pcc@...gle.com>
To: Andrey Konovalov <andreyknvl@...il.com>
Cc: Oscar Salvador <osalvador@...e.de>, Andrew Morton <akpm@...ux-foundation.org>, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	Michal Hocko <mhocko@...e.com>, Vlastimil Babka <vbabka@...e.cz>, Marco Elver <elver@...gle.com>, 
	Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH v10 1/7] lib/stackdepot: Fix first entry having a 0-handle

On Thu, Feb 15, 2024 at 3:37 PM Andrey Konovalov <andreyknvl@...il.com> wrote:
>
> On Thu, Feb 15, 2024 at 10:58 PM Oscar Salvador <osalvador@...e.de> wrote:
> >
> > The very first entry of stack_record gets a handle of 0, but this is wrong
> > because stackdepot treats a 0-handle as a non-valid one.
> > E.g: See the check in stack_depot_fetch()
> >
> > Fix this by adding and offset of 1.
> >
> > This bug has been lurking since the very beginning of stackdepot,
> > but no one really cared as it seems.
> > Because of that I am not adding a Fixes tag.
> >
> > Co-developed-by: Marco Elver <elver@...gle.com>
> > Signed-off-by: Marco Elver <elver@...gle.com>
> > Signed-off-by: Oscar Salvador <osalvador@...e.de>
> > Acked-by: Vlastimil Babka <vbabka@...e.cz>
> > ---
> >  lib/stackdepot.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> > index 4a7055a63d9f..c043a4186bc5 100644
> > --- a/lib/stackdepot.c
> > +++ b/lib/stackdepot.c
> > @@ -45,15 +45,16 @@
> >  #define DEPOT_POOL_INDEX_BITS (DEPOT_HANDLE_BITS - DEPOT_OFFSET_BITS - \
> >                                STACK_DEPOT_EXTRA_BITS)
> >  #define DEPOT_POOLS_CAP 8192
> > +/* The pool_index is offset by 1 so the first record does not have a 0 handle. */
> >  #define DEPOT_MAX_POOLS \
> > -       (((1LL << (DEPOT_POOL_INDEX_BITS)) < DEPOT_POOLS_CAP) ? \
> > -        (1LL << (DEPOT_POOL_INDEX_BITS)) : DEPOT_POOLS_CAP)
> > +       (((1LL << (DEPOT_POOL_INDEX_BITS)) - 1 < DEPOT_POOLS_CAP) ? \
> > +        (1LL << (DEPOT_POOL_INDEX_BITS)) - 1 : DEPOT_POOLS_CAP)
> >
> >  /* Compact structure that stores a reference to a stack. */
> >  union handle_parts {
> >         depot_stack_handle_t handle;
> >         struct {
> > -               u32 pool_index  : DEPOT_POOL_INDEX_BITS;
> > +               u32 pool_index  : DEPOT_POOL_INDEX_BITS; /* pool_index is offset by 1 */

Can we rename this, say to pool_index_plus_1? This will make the code
a bit clearer, as well as make it possible for debugging tools such as
drgn [1] to be able to tell when the off-by-one was introduced and
adapt accordingly.

Peter

[1] https://github.com/osandov/drgn/pull/376