Re: [PATCH 1/2] userns: Add __counted_by_ptr attribute to struct uid_gid_map

[Bill Wendling]

On Sun, Aug 23, 2026 at 7:52 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> On 08/23, Bill Wendling wrote:
> >
> > The compiler attribute __counted_by_ptr associates a pointer field of a
> > struct with a sibling field within the same struct that specifies the
> > element count of the allocated memory. This enables KASAN and fortified
> > bounds-checking to detect out-of-bounds accesses to the pointer field at
> > runtime.
> >
> > We can add the __counted_by_ptr attribute to the 'forward' and 'reverse'
> > pointer fields of 'struct uid_gid_map', which are counted by
> > 'nr_extents'. Since 'nr_extents' is defined in a sibling anonymous
> > struct inside an anonymous union, the nearest common non-anonymous
> > struct level is 'struct uid_gid_map' itself, which is supported by the
> > compiler.
> >
> > However, doing so has runtime implications. In the original
> > implementation of insert_extent(), elements are written to
> > map->forward[map->nr_extents] before map->nr_extents is incremented:
> >
> >       if (map->nr_extents < UID_GID_MAP_MAX_BASE_EXTENTS)
> >               dest = &map->extent[map->nr_extents];
> >       else
> >               dest = &map->forward[map->nr_extents];
> >
> >       *dest = *extent;
> >       map->nr_extents++;
> >
> > At the time of writing to 'map->forward[map->nr_extents]',
> > map->nr_extents is still 5, but we are accessing index 5 (which is the
> > 6th element). Under __counted_by_ptr(nr_extents), the compiler and
> > KASAN expect the accessed index to be strictly less than
> > map->nr_extents. Therefore, accessing index 5 when the count is 5
> > triggers an out-of-bounds panic/trap at runtime.
> >
> > To resolve this, insert_extent() is refactored to increment
> > map->nr_extents first, and then use map->nr_extents - 1 as the index:
> >
> >       map->nr_extents++;
> >       if (map->nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
> >               dest = &map->extent[map->nr_extents - 1];
> >       else
> >               dest = &map->forward[map->nr_extents - 1];
> >
> >       *dest = *extent;
>
> I leave this to you and other reviewers (add Alexey), you can safely
> ignore my nit.
>
> To me
>
>         if (map->nr_extents < UID_GID_MAP_MAX_BASE_EXTENTS)
>                 dest = &map->extent;
>         else
>                 dest = &map->forward;
>
>         map->nr_extents++;
>         dest[map->nr_extents - 1] = *extent;
>
> looks a bit more clear, but this is minor/subjective.
>
> Either way, I think this needs a short comment to explain why do we
> need to increment ->nr_extents first, then subtract 1. IOW, to explain
> why (say)
>
>         dest[map->nr_extents++] = *extent;
>
> would be wrong.
>
Hi Oleg,

I added a comment before the code in question explaining why it's
organized that way. I certainly don't hate your version, but for
simplicity of reviews I just left the code as is.

-bw

> Oleg.
>
> > Assisted-by: Gemini Next
> > Signed-off-by: Bill Wendling <morbo@xxxxxxxxxx>
> > ---
> > Cc: Kees Cook <kees@xxxxxxxxxx>
> > Cc: "Gustavo A. R. Silva" <gustavoars@xxxxxxxxxx>
> > Cc: Christian Brauner <brauner@xxxxxxxxxx>
> > Cc: Aleksa Sarai <cyphar@xxxxxxxxxx>
> > Cc: Jan Kara <jack@xxxxxxx>
> > Cc: Nathan Chancellor <nathan@xxxxxxxxxx>
> > Cc: Miguel Ojeda <ojeda@xxxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxx>
> > Cc: Nicolas Schier <nsc@xxxxxxxxxx>
> > Cc: Gary Guo <gary@xxxxxxxxxxx>
> > Cc: "Thomas Weißschuh" <thomas.weissschuh@xxxxxxxxxxxxx>
> > Cc: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> > Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > Cc: Anand Moon <linux.amoon@xxxxxxxxx>
> > Cc: Oleg Nesterov <oleg@xxxxxxxxxx>
> > Cc: codemender-patching+linux@xxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: linux-hardening@xxxxxxxxxxxxxxx
> > ---
> >  include/linux/user_namespace.h | 4 ++--
> >  kernel/user_namespace.c        | 8 ++++----
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> > index e38d9e60569f..2962256eddf7 100644
> > --- a/include/linux/user_namespace.h
> > +++ b/include/linux/user_namespace.h
> > @@ -29,8 +29,8 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */
> >                       u32 nr_extents;
> >               };
> >               struct {
> > -                     struct uid_gid_extent *forward;
> > -                     struct uid_gid_extent *reverse;
> > +                     struct uid_gid_extent *forward __counted_by_ptr(nr_extents);
> > +                     struct uid_gid_extent *reverse __counted_by_ptr(nr_extents);
> >               };
> >       };
> >  };
> > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> > index 0bed462e9b2a..7e5371d8f515 100644
> > --- a/kernel/user_namespace.c
> > +++ b/kernel/user_namespace.c
> > @@ -809,13 +809,13 @@ static int insert_extent(struct uid_gid_map *map, struct uid_gid_extent *extent)
> >               map->reverse = NULL;
> >       }
> >
> > -     if (map->nr_extents < UID_GID_MAP_MAX_BASE_EXTENTS)
> > -             dest = &map->extent[map->nr_extents];
> > +     map->nr_extents++;
> > +     if (map->nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
> > +             dest = &map->extent[map->nr_extents - 1];
> >       else
> > -             dest = &map->forward[map->nr_extents];
> > +             dest = &map->forward[map->nr_extents - 1];
> >
> >       *dest = *extent;
> > -     map->nr_extents++;
> >       return 0;
> >  }
> >
> > --
> > 2.55.0.860.g4b6b3295ed-goog
> >
>