| Message ID | 20220504014440.3697851-29-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Introduce flexible array struct memcpy() helpers | expand |
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)
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.
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.
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...
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.
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 --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);
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(-)