diff mbox series

lsm: Resolve compiling 'security.c' error

Message ID 20240117014541.8887-1-yaolu@kylinos.cn
State New
Headers
Series lsm: Resolve compiling 'security.c' error |

Commit Message

Lu Yao Jan. 17, 2024, 1:45 a.m. UTC 
  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)'

Signed-off-by: Lu Yao <yaolu@kylinos.cn>
---
 security/security.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paul Moore Jan. 17, 2024, 2:32 p.m. UTC | #1 
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)'
>
> 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?

> 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;
> --
> 2.25.1
Kees Cook Jan. 17, 2024, 8:51 p.m. UTC | #2 
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;
 	}
Paul Moore Jan. 17, 2024, 10:03 p.m. UTC | #3 
On Wed, Jan 17, 2024 at 3:51 PM Kees Cook <keescook@chromium.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@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.

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
diff mbox series

Patch

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;