linux-hardening - Re: [PATCH] lsm: Resolve compiling 'security.c' error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Wed, 17 Jan 2024 17:03:14 -0500
From: Paul Moore <paul@...l-moore.com>
To: Kees Cook <keescook@...omium.org>
Cc: Lu Yao <yaolu@...inos.cn>, linux-hardening@...r.kernel.org, jmorris@...ei.org, 
	serge@...lyn.com, linux-security-module@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lsm: Resolve compiling 'security.c' error

On Wed, Jan 17, 2024 at 3:51 PM Kees Cook <keescook@...omium.org> wrote:
> On Wed, Jan 17, 2024 at 09:32:33AM -0500, Paul Moore wrote:
> > On Tue, Jan 16, 2024 at 8:46 PM Lu Yao <yaolu@...inos.cn> wrote:
> > >
> > > The following error log is displayed during the current compilation
> > >   > 'security/security.c:810:2: error: ‘memcpy’ offset 32 is
> > >   > out of the bounds [0, 0] [-Werror=array-bounds]'
> > >
> > > GCC version is '10.3.0 (Ubuntu 10.3.0-1ubuntu1~18.04~1)'
>
> As an aside, Ubuntu 18.04 went out of support back in June 2023, and
> never officially supported gcc 10:
> https://launchpad.net/ubuntu/+source/gcc-10
>
> That said, I still see this error with gcc 10.5 on supported Ubuntu
> releases. I'm surprised this is the first time I've seen this error!
>
> > >
> > > Signed-off-by: Lu Yao <yaolu@...inos.cn>
> > > ---
> > >  security/security.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > I'm adding the linux-hardening folks to the to To: line as this has
> > now come up multiple times and my best guess is that this is an issue
> > with the struct_size() macro, compiler annotations, or something
> > similar and I suspect they are the experts in that area.  My
> > understanding is that using the struct_size() macro is preferable to
> > open coding the math, as this patch does, but if we have to do
> > something like this to silence the warnings, that's okay with me.
> >
> > So linux-hardening folks, what do you say?
>
> This is a GCC bug -- it thinks nctx_len could be 0, so it gets mad about
> the array bounds.

Ah ha, thanks for that.

> > > diff --git a/security/security.c b/security/security.c
> > > index 0144a98d3712..37168f6bee25 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -792,7 +792,7 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
> > >         size_t nctx_len;
> > >         int rc = 0;
> > >
> > > -       nctx_len = ALIGN(struct_size(nctx, ctx, val_len), sizeof(void *));
> > > +       nctx_len = ALIGN(sizeof(struct lsm_ctx) + val_len, sizeof(void *));
> > >         if (nctx_len > *uctx_len) {
> > >                 rc = -E2BIG;
> > >                 goto out;
>
> Please don't do this -- it regresses the efforts to make sure we can
> never wrap the math on here.

I didn't want to apply that change, hence my To/CC to you guys.  If
there was no other option to silence the warning then we would
probably have to do it, but it looks like we have options.

> We need to pick one of these two diffs
> instead. The first disables -Warray-bounds for GCC 10.* also (since we
> keep having false positives). The latter silences this 1 particular
> case by explicitly checking nctx_len for 0:
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 8d4e836e1b6b..af4833430aca 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -874,7 +874,7 @@ config GCC11_NO_ARRAY_BOUNDS
>
>  config CC_NO_ARRAY_BOUNDS
>         bool
> -       default y if CC_IS_GCC && GCC_VERSION >= 110000 && GCC11_NO_ARRAY_BOUNDS
> +       default y if CC_IS_GCC && GCC_VERSION >= 100000 && GCC11_NO_ARRAY_BOUNDS
>
>  # Currently, disable -Wstringop-overflow for GCC 11, globally.
>  config GCC11_NO_STRINGOP_OVERFLOW

It sounds like lsm_fill_user_ctx() is not the only one having problems
with GCC v10 so I'm guessing you're going to submit a patch like the
above up to Linus?  I think that's preferable to adding extra checks
we don't really need just to silence a buggy compiler.

> diff --git a/security/security.c b/security/security.c
> index 0144a98d3712..ab403136958f 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -793,7 +793,7 @@ int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
>         int rc = 0;
>
>         nctx_len = ALIGN(struct_size(nctx, ctx, val_len), sizeof(void *));
> -       if (nctx_len > *uctx_len) {
> +       if (nctx_len == 0 || nctx_len > *uctx_len) {
>                 rc = -E2BIG;
>                 goto out;
>         }
>
> --
> Kees Cook

-- 
paul-moore.com