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

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.

Oleg.

> Assisted-by: Gemini Next
> Signed-off-by: Bill Wendling <[email protected]>
> ---
> Cc: Kees Cook <[email protected]>
> Cc: "Gustavo A. R. Silva" <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: Aleksa Sarai <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: Nathan Chancellor <[email protected]>
> Cc: Miguel Ojeda <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Nicolas Schier <[email protected]>
> Cc: Gary Guo <[email protected]>
> Cc: "Thomas Weißschuh" <[email protected]>
> Cc: Alice Ryhl <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Anand Moon <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
>  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
>