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

[Kees Cook <keescook@chromium.org>]

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@kylinos.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@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.

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



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