Re: [PATCH 28/32] selinux: Use mem_to_flex_dup() with xfrm and sidtab

Paul Moore
2022-05-04T23:29:04Z

On Tue, May 3, 2022 at 9:57 PM Kees Cook <keescook@chromium.org> wrote:
>
> As part of the work to perform bounds checking on all memcpy() uses,
> replace the open-coded a deserialization of bytes out of memory into a
> trailing flexible array by using a flex_array.h helper to perform the
> allocation, bounds checking, and copying:
>
> struct xfrm_sec_ctx
> struct sidtab_str_cache
>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <stephen.smalley.work@gmail.com>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Xiu Jianfeng <xiujianfeng@huawei.com>
> Cc: "Christian Göttsche" <cgzones@googlemail.com>
> Cc: netdev@vger.kernel.org
> Cc: selinux@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> include/uapi/linux/xfrm.h | 4 ++--
> security/selinux/ss/sidtab.c | 9 +++------
> security/selinux/xfrm.c | 7 ++-----
> 3 files changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> index 65e13a099b1a..4a6fa2beff6a 100644
> --- a/include/uapi/linux/xfrm.h
> +++ b/include/uapi/linux/xfrm.h
> @@ -31,9 +31,9 @@ struct xfrm_id {
> struct xfrm_sec_ctx {
> __u8 ctx_doi;
> __u8 ctx_alg;
> - __u16 ctx_len;
> + __DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(__u16, ctx_len);
> __u32 ctx_sid;
> - char ctx_str[0];
> + __DECLARE_FLEX_ARRAY_ELEMENTS(char, ctx_str);
> };

While I like the idea of this in principle, I'd like to hear about the
testing you've done on these patches. A previous flex array
conversion in the audit uapi headers ended up causing a problem with
GCC12 and SWIG; while it was a SWIG problem and not a kernel header
problem that was thin consolation for those with broken builds.

> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
> index a54b8652bfb5..a9d434e8cff7 100644
> --- a/security/selinux/ss/sidtab.c
> +++ b/security/selinux/ss/sidtab.c
> @@ -23,8 +23,8 @@ struct sidtab_str_cache {
> struct rcu_head rcu_member;
> struct list_head lru_member;
> struct sidtab_entry *parent;
> - u32 len;
> - char str[];
> + DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, len);
> + DECLARE_FLEX_ARRAY_ELEMENTS(char, str);
> };
>
> #define index_to_sid(index) ((index) + SECINITSID_NUM + 1)

--
paul-moore.com