Received-SPF: pass (google.com: domain of linux-kernel+bounces-29477-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240117014541.8887-1-yaolu@kylinos.cn> <CAHC9VhSs6bxXFCAhw7i5cN=iZtuG3-E8xDBRjyGsop=BrhbmSw@mail.gmail.com>
 <202401171236.5175FA9FA9@keescook>
In-Reply-To: <202401171236.5175FA9FA9@keescook>
From: Paul Moore <paul@paul-moore.com>
Date: Wed, 17 Jan 2024 17:03:14 -0500
Message-ID: <CAHC9VhR+d70b6QfAdtoch-M5cttM63KpMcKG-tfv5PZB9=bnSg@mail.gmail.com>
Subject: Re: [PATCH] lsm: Resolve compiling 'security.c' error
To: Kees Cook <keescook@chromium.org>
Cc: Lu Yao <yaolu@kylinos.cn>, linux-hardening@vger.kernel.org, jmorris@namei.org, 
	serge@hallyn.com, linux-security-module@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 17, 2024 at 3:51=E2=80=AFPM Kees Cook <keescook@chromium.org> w=
rote:
> On Wed, Jan 17, 2024 at 09:32:33AM -0500, Paul Moore wrote:
> > On Tue, Jan 16, 2024 at 8:46=E2=80=AFPM Lu Yao <yaolu@kylinos.cn> wrote=
:
> > >
> > > The following error log is displayed during the current compilation
> > >   > 'security/security.c:810:2: error: =E2=80=98memcpy=E2=80=99 offse=
t 32 is
> > >   > out of the bounds [0, 0] [-Werror=3Darray-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@kylinos.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 =3D 0;
> > >
> > > -       nctx_len =3D ALIGN(struct_size(nctx, ctx, val_len), sizeof(vo=
id *));
> > > +       nctx_len =3D ALIGN(sizeof(struct lsm_ctx) + val_len, sizeof(v=
oid *));
> > >         if (nctx_len > *uctx_len) {
> > >                 rc =3D -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 >=3D 110000 && GCC11_NO_ARR=
AY_BOUNDS
> +       default y if CC_IS_GCC && GCC_VERSION >=3D 100000 && GCC11_NO_ARR=
AY_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, si=
ze_t *uctx_len,
>         int rc =3D 0;
>
>         nctx_len =3D ALIGN(struct_size(nctx, ctx, val_len), sizeof(void *=
));
> -       if (nctx_len > *uctx_len) {
> +       if (nctx_len =3D=3D 0 || nctx_len > *uctx_len) {
>                 rc =3D -E2BIG;
>                 goto out;
>         }
>
> --
> Kees Cook

--=20
paul-moore.com