diff mbox series

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

Message ID 20220504014440.3697851-29-keescook@chromium.org (mailing list archive)
State Handled Elsewhere
Headers show
Series Introduce flexible array struct memcpy() helpers | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint success Gitlint PASS
tedd_an/subjectprefix fail "Bluetooth: " is not specified in the subject

Commit Message

Kees Cook May 4, 2022, 1:44 a.m. UTC
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(-)

Comments

Paul Moore May 4, 2022, 10:57 p.m. UTC | #1
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)
Gustavo A. R. Silva May 4, 2022, 11:43 p.m. UTC | #2
Hi Paul,

On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote:
> On Tue, May 3, 2022 at 9:57 PM Kees Cook <keescook@chromium.org> wrote:

[..]

> > +++ 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

I'm curious about which commit caused those problems...?

Thanks
--
Gustavo

> GCC12 and SWIG; while it was a SWIG problem and not a kernel header
> problem that was thin consolation for those with broken builds.
Paul Moore May 5, 2022, 3:14 a.m. UTC | #3
On Wed, May 4, 2022 at 7:34 PM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> Hi Paul,
>
> On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote:
> > On Tue, May 3, 2022 at 9:57 PM Kees Cook <keescook@chromium.org> wrote:
>
> [..]
>
> > > +++ 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
>
> I'm curious about which commit caused those problems...?

Commit ed98ea2128b6 ("audit: replace zero-length array with
flexible-array member"), however, as I said earlier, the problem was
actually with SWIG, it just happened to be triggered by the kernel
commit.  There was a brief fedora-devel mail thread about the problem,
see the link below:

* https://www.spinics.net/lists/fedora-devel/msg297991.html

To reiterate, I'm supportive of changes like this, but I would like to
hear how it was tested to ensure there are no unexpected problems with
userspace.  If there are userspace problems it doesn't mean we can't
make changes like this, it just means we need to ensure that the
userspace issues are resolved first.
Kees Cook May 5, 2022, 6:39 p.m. UTC | #4
On Wed, May 04, 2022 at 11:14:42PM -0400, Paul Moore wrote:
> On Wed, May 4, 2022 at 7:34 PM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
> >
> > Hi Paul,
> >
> > On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote:
> > > On Tue, May 3, 2022 at 9:57 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > [..]
> >
> > > > +++ 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
> >
> > I'm curious about which commit caused those problems...?
> 
> Commit ed98ea2128b6 ("audit: replace zero-length array with
> flexible-array member"), however, as I said earlier, the problem was
> actually with SWIG, it just happened to be triggered by the kernel
> commit.  There was a brief fedora-devel mail thread about the problem,
> see the link below:
> 
> * https://www.spinics.net/lists/fedora-devel/msg297991.html

Wow, that's pretty weird -- it looks like SWIG was scraping the headers
to build its conversions? I assume SWIG has been fixed now?

> To reiterate, I'm supportive of changes like this, but I would like to
> hear how it was tested to ensure there are no unexpected problems with
> userspace.  If there are userspace problems it doesn't mean we can't
> make changes like this, it just means we need to ensure that the
> userspace issues are resolved first.

Well, as this is the first and only report of any problems with [0] -> []
conversions (in UAPI or anywhere) that I remember seeing, and they've
been underway since at least v5.9, I hadn't been doing any new testing.

So, for this case, I guess I should ask what tests you think would be
meaningful here? Anything using #include should be fine:
https://codesearch.debian.net/search?q=linux%2Fxfrm.h&literal=1&perpkg=1
Which leaves just this, which may be doing something weird:

libabigail_2.0-1/tests/data/test-diff-filter/test-PR27569-v0.abi
        </data-member>
        <data-member access="public" layout-offset-in-bits="128">
          <var-decl name="seq_hi" type-id="3f1a6b60" visibility="default" filepath="include/uapi/linux/xfrm.h" line="97" column="1"/>
        </data-member>
        <data-member access="public" layout-offset-in-bits="160">

But I see that SWIG doesn't show up in a search for linux/audit.h:
https://codesearch.debian.net/search?q=linux%2Faudit.h&literal=1&perpkg=1

So this may not be a sufficient analysis...
Paul Moore May 5, 2022, 11:16 p.m. UTC | #5
On Thu, May 5, 2022 at 2:39 PM Kees Cook <keescook@chromium.org> wrote:
> On Wed, May 04, 2022 at 11:14:42PM -0400, Paul Moore wrote:
> > On Wed, May 4, 2022 at 7:34 PM Gustavo A. R. Silva
> > <gustavoars@kernel.org> wrote:
> > >
> > > Hi Paul,
> > >
> > > On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote:
> > > > On Tue, May 3, 2022 at 9:57 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > [..]
> > >
> > > > > +++ 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
> > >
> > > I'm curious about which commit caused those problems...?
> >
> > Commit ed98ea2128b6 ("audit: replace zero-length array with
> > flexible-array member"), however, as I said earlier, the problem was
> > actually with SWIG, it just happened to be triggered by the kernel
> > commit.  There was a brief fedora-devel mail thread about the problem,
> > see the link below:
> >
> > * https://www.spinics.net/lists/fedora-devel/msg297991.html
>
> Wow, that's pretty weird -- it looks like SWIG was scraping the headers
> to build its conversions? I assume SWIG has been fixed now?

I honestly don't know, the audit userspace was hacking around it with
some header file duplication/munging last I heard, but I try to avoid
having to touch Steve's audit userspace code.

> > To reiterate, I'm supportive of changes like this, but I would like to
> > hear how it was tested to ensure there are no unexpected problems with
> > userspace.  If there are userspace problems it doesn't mean we can't
> > make changes like this, it just means we need to ensure that the
> > userspace issues are resolved first.
>
> Well, as this is the first and only report of any problems with [0] -> []
> conversions (in UAPI or anywhere) that I remember seeing, and they've
> been underway since at least v5.9, I hadn't been doing any new testing.

... and for whatever it is worth, I wasn't expecting it to be a
problem either.  Surprise :)

> So, for this case, I guess I should ask what tests you think would be
> meaningful here? Anything using #include should be fine:
> https://codesearch.debian.net/search?q=linux%2Fxfrm.h&literal=1&perpkg=1
> Which leaves just this, which may be doing something weird:
>
> libabigail_2.0-1/tests/data/test-diff-filter/test-PR27569-v0.abi
>         </data-member>
>         <data-member access="public" layout-offset-in-bits="128">
>           <var-decl name="seq_hi" type-id="3f1a6b60" visibility="default" filepath="include/uapi/linux/xfrm.h" line="97" column="1"/>
>         </data-member>
>         <data-member access="public" layout-offset-in-bits="160">
>
> But I see that SWIG doesn't show up in a search for linux/audit.h:
> https://codesearch.debian.net/search?q=linux%2Faudit.h&literal=1&perpkg=1
>
> So this may not be a sufficient analysis...

I think from a practical perspective ensuring that the major IPsec/IKE
tools, e.g. the various *SWANs, that know about labeled IPSec still
build and can set/get the SA/SPD labels correctly would be sufficient.
I seriously doubt there would be any problems, but who knows.
Gustavo A. R. Silva May 6, 2022, 1:08 a.m. UTC | #6
On Thu, May 05, 2022 at 07:16:18PM -0400, Paul Moore wrote:
> On Thu, May 5, 2022 at 2:39 PM Kees Cook <keescook@chromium.org> wrote:
> > On Wed, May 04, 2022 at 11:14:42PM -0400, Paul Moore wrote:
> > > On Wed, May 4, 2022 at 7:34 PM Gustavo A. R. Silva
> > > <gustavoars@kernel.org> wrote:
> > > >
> > > > Hi Paul,
> > > >
> > > > On Wed, May 04, 2022 at 06:57:28PM -0400, Paul Moore wrote:
> > > > > On Tue, May 3, 2022 at 9:57 PM Kees Cook <keescook@chromium.org> wrote:
> > > >
> > > > [..]
> > > >
> > > > > > +++ 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
> > > >
> > > > I'm curious about which commit caused those problems...?
> > >
> > > Commit ed98ea2128b6 ("audit: replace zero-length array with
> > > flexible-array member"), however, as I said earlier, the problem was
> > > actually with SWIG, it just happened to be triggered by the kernel
> > > commit.  There was a brief fedora-devel mail thread about the problem,
> > > see the link below:
> > >
> > > * https://www.spinics.net/lists/fedora-devel/msg297991.html
> >
> > Wow, that's pretty weird -- it looks like SWIG was scraping the headers
> > to build its conversions? I assume SWIG has been fixed now?
> 
> I honestly don't know, the audit userspace was hacking around it with
> some header file duplication/munging last I heard, but I try to avoid
> having to touch Steve's audit userspace code.
> 
> > > To reiterate, I'm supportive of changes like this, but I would like to
> > > hear how it was tested to ensure there are no unexpected problems with
> > > userspace.  If there are userspace problems it doesn't mean we can't
> > > make changes like this, it just means we need to ensure that the
> > > userspace issues are resolved first.
> >
> > Well, as this is the first and only report of any problems with [0] -> []
> > conversions (in UAPI or anywhere) that I remember seeing, and they've
> > been underway since at least v5.9, I hadn't been doing any new testing.
> 
> ... and for whatever it is worth, I wasn't expecting it to be a
> problem either.  Surprise :)
> 
> > So, for this case, I guess I should ask what tests you think would be
> > meaningful here? Anything using #include should be fine:
> > https://codesearch.debian.net/search?q=linux%2Fxfrm.h&literal=1&perpkg=1
> > Which leaves just this, which may be doing something weird:
> >
> > libabigail_2.0-1/tests/data/test-diff-filter/test-PR27569-v0.abi
> >         </data-member>
> >         <data-member access="public" layout-offset-in-bits="128">
> >           <var-decl name="seq_hi" type-id="3f1a6b60" visibility="default" filepath="include/uapi/linux/xfrm.h" line="97" column="1"/>
> >         </data-member>
> >         <data-member access="public" layout-offset-in-bits="160">
> >
> > But I see that SWIG doesn't show up in a search for linux/audit.h:
> > https://codesearch.debian.net/search?q=linux%2Faudit.h&literal=1&perpkg=1
> >
> > So this may not be a sufficient analysis...
> 
> I think from a practical perspective ensuring that the major IPsec/IKE
> tools, e.g. the various *SWANs, that know about labeled IPSec still
> build and can set/get the SA/SPD labels correctly would be sufficient.
> I seriously doubt there would be any problems, but who knows.

There are certainly some cases in which the transformation of
zero-length arrays into flexible-array members can bring some issues
to the surface[1][2]. This is the first time that we know of one of
them in user-space. However, we haven't transformed the arrays in
UAPI yet (with the exception of a couple of cases[3][4]). But that
is something that we are planning to try soon[5].

--
Gustavo

[1] https://github.com/KSPP/linux/issues?q=invalid+use+of+flexible+array
[2] https://github.com/KSPP/linux/issues?q=invalid+application+of+%E2%80%98sizeof%E2%80%99+to+incomplete+type
[3] https://git.kernel.org/linus/db243b796439c0caba47865564d8acd18a301d18
[4] https://git.kernel.org/linus/d6cdad870358128c1e753e6258e295ab8a5a2429
[5] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?h=for-next/kspp-fam0-uapi
diff mbox series

Patch

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);
 };
 
 /* Security Context Domains of Interpretation */
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)
@@ -570,8 +570,7 @@  void sidtab_sid2str_put(struct sidtab *s, struct sidtab_entry *entry,
 		goto out_unlock;
 	}
 
-	cache = kmalloc(struct_size(cache, str, str_len), GFP_ATOMIC);
-	if (!cache)
+	if (mem_to_flex_dup(&cache, str, str_len, GFP_ATOMIC))
 		goto out_unlock;
 
 	if (s->cache_free_slots == 0) {
@@ -584,8 +583,6 @@  void sidtab_sid2str_put(struct sidtab *s, struct sidtab_entry *entry,
 		s->cache_free_slots--;
 	}
 	cache->parent = entry;
-	cache->len = str_len;
-	memcpy(cache->str, str, str_len);
 	list_add(&cache->lru_member, &s->cache_lru_list);
 
 	rcu_assign_pointer(entry->cache, cache);
diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
index c576832febc6..bc7a54bf8f0d 100644
--- a/security/selinux/xfrm.c
+++ b/security/selinux/xfrm.c
@@ -345,7 +345,7 @@  int selinux_xfrm_state_alloc_acquire(struct xfrm_state *x,
 				     struct xfrm_sec_ctx *polsec, u32 secid)
 {
 	int rc;
-	struct xfrm_sec_ctx *ctx;
+	struct xfrm_sec_ctx *ctx = NULL;
 	char *ctx_str = NULL;
 	u32 str_len;
 
@@ -360,8 +360,7 @@  int selinux_xfrm_state_alloc_acquire(struct xfrm_state *x,
 	if (rc)
 		return rc;
 
-	ctx = kmalloc(struct_size(ctx, ctx_str, str_len), GFP_ATOMIC);
-	if (!ctx) {
+	if (mem_to_flex_dup(&ctx, ctx_str, str_len, GFP_ATOMIC)) {
 		rc = -ENOMEM;
 		goto out;
 	}
@@ -369,8 +368,6 @@  int selinux_xfrm_state_alloc_acquire(struct xfrm_state *x,
 	ctx->ctx_doi = XFRM_SC_DOI_LSM;
 	ctx->ctx_alg = XFRM_SC_ALG_SELINUX;
 	ctx->ctx_sid = secid;
-	ctx->ctx_len = str_len;
-	memcpy(ctx->ctx_str, ctx_str, str_len);
 
 	x->security = ctx;
 	atomic_inc(&selinux_xfrm_refcount);