LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] ovl: explicitly initialize error in ovl_copy_xattr()
@ 2020-06-03 17:47 glider
  2020-06-03 20:35 ` Kees Cook
  2020-06-03 21:46 ` Vivek Goyal
  0 siblings, 2 replies; 10+ messages in thread
From: glider @ 2020-06-03 17:47 UTC (permalink / raw)
  To: miklos
  Cc: linux-unionfs, linux-kernel, keescook, royyang, stable,
	Alexander Potapenko

Under certain circumstances (we found this out running Docker on a
Clang-built kernel with CONFIG_INIT_STACK_ALL) ovl_copy_xattr() may
return uninitialized value of |error| from ovl_copy_xattr().
It is then returned by ovl_create() to lookup_open(), which casts it to
an invalid dentry pointer, that can be further read or written by the
lookup_open() callers.

Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Roy Yang <royyang@google.com>
Cc: <stable@vger.kernel.org> # 4.1

---

It's unclear to me whether error should be initially 0 or some error
code (both seem to work), but I thought returning an error makes sense,
as the situation wasn't anticipated by the code authors.

The bug seem to date back to at least v4.1 where the annotation has been
introduced (i.e. the compilers started noticing error could be used
before being initialized). I hovever didn't try to prove that the
problem is actually reproducible on such ancient kernels. We've seen it
on a real machine running v4.4 as well.
---
 fs/overlayfs/copy_up.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 9709cf22cab3..428d43e2d016 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -47,7 +47,7 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
 {
 	ssize_t list_size, size, value_size = 0;
 	char *buf, *name, *value = NULL;
-	int uninitialized_var(error);
+	int error = -EINVAL;
 	size_t slen;
 
 	if (!(old->d_inode->i_opflags & IOP_XATTR) ||
-- 
2.27.0.rc2.251.g90737beb825-goog


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ovl: explicitly initialize error in ovl_copy_xattr()
  2020-06-03 17:47 [PATCH] ovl: explicitly initialize error in ovl_copy_xattr() glider
@ 2020-06-03 20:35 ` Kees Cook
  2020-06-03 21:46 ` Vivek Goyal
  1 sibling, 0 replies; 10+ messages in thread
From: Kees Cook @ 2020-06-03 20:35 UTC (permalink / raw)
  To: glider; +Cc: miklos, linux-unionfs, linux-kernel, royyang, stable

On Wed, Jun 03, 2020 at 07:47:14PM +0200, glider@google.com wrote:
> Under certain circumstances (we found this out running Docker on a
> Clang-built kernel with CONFIG_INIT_STACK_ALL) ovl_copy_xattr() may
> return uninitialized value of |error| from ovl_copy_xattr().
> It is then returned by ovl_create() to lookup_open(), which casts it to
> an invalid dentry pointer, that can be further read or written by the
> lookup_open() callers.
> 
> Signed-off-by: Alexander Potapenko <glider@google.com>

Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1050405
Fixes: e4ad29fa0d22 ("ovl: use a minimal buffer in ovl_copy_xattr")
Cc: stable@vger.kernel.org
Reviewed-by: Kees Cook <keescook@chromium.org>

It seems the error isn't reported anywhere, so the value likely isn't
too important. -EINVAL seems sane to me.

Thought: should CONFIG_INIT_STACK_ALL=y disable uninitialized_var()?

$ git grep uninitialized_var | wc -l
300

We have evidence this is being used inappropriately and is masking bugs.
I would actually think it should should be removed globally, but it
seems especially important for CONFIG_INIT_STACK_ALL=y.

I've opened:
https://github.com/KSPP/linux/issues/81

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ovl: explicitly initialize error in ovl_copy_xattr()
  2020-06-03 17:47 [PATCH] ovl: explicitly initialize error in ovl_copy_xattr() glider
  2020-06-03 20:35 ` Kees Cook
@ 2020-06-03 21:46 ` Vivek Goyal
  2020-06-04  8:28   ` Alexander Potapenko
  1 sibling, 1 reply; 10+ messages in thread
From: Vivek Goyal @ 2020-06-03 21:46 UTC (permalink / raw)
  To: glider; +Cc: miklos, linux-unionfs, linux-kernel, keescook, royyang, stable

On Wed, Jun 03, 2020 at 07:47:14PM +0200, glider@google.com wrote:
> Under certain circumstances (we found this out running Docker on a
> Clang-built kernel with CONFIG_INIT_STACK_ALL) ovl_copy_xattr() may
> return uninitialized value of |error| from ovl_copy_xattr().

If we are returning uninitialized value of error, doesn't that mean
that somewhere in the function we are returning without setting error.
And that probably means that's a bug and we should fix it?

I am wondering if this is triggered by loop finishing because all
the xattr on the file are ovl_is_private_xattr(). In that case, we
will come out of the loop without setting error. This is in fact
success and we should return 0 instead of some random error?

Thanks
Vivek


> It is then returned by ovl_create() to lookup_open(), which casts it to
> an invalid dentry pointer, that can be further read or written by the
> lookup_open() callers.
> 
> Signed-off-by: Alexander Potapenko <glider@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Roy Yang <royyang@google.com>
> Cc: <stable@vger.kernel.org> # 4.1
> 
> ---
> 
> It's unclear to me whether error should be initially 0 or some error
> code (both seem to work), but I thought returning an error makes sense,
> as the situation wasn't anticipated by the code authors.
> 
> The bug seem to date back to at least v4.1 where the annotation has been
> introduced (i.e. the compilers started noticing error could be used
> before being initialized). I hovever didn't try to prove that the
> problem is actually reproducible on such ancient kernels. We've seen it
> on a real machine running v4.4 as well.
> ---
>  fs/overlayfs/copy_up.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 9709cf22cab3..428d43e2d016 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -47,7 +47,7 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
>  {
>  	ssize_t list_size, size, value_size = 0;
>  	char *buf, *name, *value = NULL;
> -	int uninitialized_var(error);
> +	int error = -EINVAL;
>  	size_t slen;
>  
>  	if (!(old->d_inode->i_opflags & IOP_XATTR) ||
> -- 
> 2.27.0.rc2.251.g90737beb825-goog
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ovl: explicitly initialize error in ovl_copy_xattr()
  2020-06-03 21:46 ` Vivek Goyal
@ 2020-06-04  8:28   ` Alexander Potapenko
  0 siblings, 0 replies; 10+ messages in thread
From: Alexander Potapenko @ 2020-06-04  8:28 UTC (permalink / raw)
  To: Vivek Goyal; +Cc: miklos, linux-unionfs, LKML, Kees Cook, Roy Yang, stable

On Wed, Jun 3, 2020 at 11:46 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Wed, Jun 03, 2020 at 07:47:14PM +0200, glider@google.com wrote:
> > Under certain circumstances (we found this out running Docker on a
> > Clang-built kernel with CONFIG_INIT_STACK_ALL) ovl_copy_xattr() may
> > return uninitialized value of |error| from ovl_copy_xattr().
>
> If we are returning uninitialized value of error, doesn't that mean
> that somewhere in the function we are returning without setting error.
> And that probably means that's a bug and we should fix it?

Could be. My understanding of that code is quite limited, so I'm happy
to change the patch if necessary.

> I am wondering if this is triggered by loop finishing because all
> the xattr on the file are ovl_is_private_xattr().

Yes, that's the case. The loop makes one iteration, then
ovl_is_private_xattr() returns true, then the loop ends.

> In that case, we
> will come out of the loop without setting error. This is in fact
> success and we should return 0 instead of some random error?

Thanks for letting me know. I'll change that to 0 then.

> Thanks
> Vivek
>
>
> > It is then returned by ovl_create() to lookup_open(), which casts it to
> > an invalid dentry pointer, that can be further read or written by the
> > lookup_open() callers.
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Roy Yang <royyang@google.com>
> > Cc: <stable@vger.kernel.org> # 4.1
> >
> > ---
> >
> > It's unclear to me whether error should be initially 0 or some error
> > code (both seem to work), but I thought returning an error makes sense,
> > as the situation wasn't anticipated by the code authors.
> >
> > The bug seem to date back to at least v4.1 where the annotation has been
> > introduced (i.e. the compilers started noticing error could be used
> > before being initialized). I hovever didn't try to prove that the
> > problem is actually reproducible on such ancient kernels. We've seen it
> > on a real machine running v4.4 as well.
> > ---
> >  fs/overlayfs/copy_up.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 9709cf22cab3..428d43e2d016 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -47,7 +47,7 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
> >  {
> >       ssize_t list_size, size, value_size = 0;
> >       char *buf, *name, *value = NULL;
> > -     int uninitialized_var(error);
> > +     int error = -EINVAL;
> >       size_t slen;
> >
> >       if (!(old->d_inode->i_opflags & IOP_XATTR) ||
> > --
> > 2.27.0.rc2.251.g90737beb825-goog
> >
>


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ovl: explicitly initialize error in ovl_copy_xattr()
  2020-06-04 15:57 ` Kees Cook
  2020-06-05  9:46   ` Alexander Potapenko
@ 2020-06-05  9:54   ` Miklos Szeredi
  1 sibling, 0 replies; 10+ messages in thread
From: Miklos Szeredi @ 2020-06-05  9:54 UTC (permalink / raw)
  To: Kees Cook
  Cc: Alexander Potapenko, Vivek Goyal, overlayfs, linux-kernel,
	royyang, stable

On Thu, Jun 4, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Jun 04, 2020 at 10:42:45AM +0200, glider@google.com wrote:
> > Under certain circumstances (we found this out running Docker on a
> > Clang-built kernel with CONFIG_INIT_STACK_ALL) ovl_copy_xattr() may
> > return uninitialized value of |error| from ovl_copy_xattr().
> > It is then returned by ovl_create() to lookup_open(), which casts it to
> > an invalid dentry pointer, that can be further read or written by the
> > lookup_open() callers.
> >
> > The uninitialized value is returned when all the xattr on the file
> > are ovl_is_private_xattr(), which is actually a successful case,
> > therefore we initialize |error| with 0.
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Roy Yang <royyang@google.com>
> > Cc: <stable@vger.kernel.org> # 4.1
>
> Please include a Fixes (more below) and Link tags for details to help
> guide backporting, then you don't need to bother with with "# 4.1",
> the -stable tools will figure it out with a "Fixes" tag.
>
> Thanks for the v2!
>
> Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1050405
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> > The bug seem to date back to at least v4.1 where the annotation has been
> > introduced (i.e. the compilers started noticing error could be used
> > before being initialized). I hovever didn't try to prove that the
> > problem is actually reproducible on such ancient kernels. We've seen it
> > on a real machine running v4.4 as well.
>
> It seems like it came from this, but that's v4.5:
>
> Fixes: e4ad29fa0d22 ("ovl: use a minimal buffer in ovl_copy_xattr")


I believe it's actually:

Fixes: 0956254a2d5b ("ovl: don't copy up opaqueness")

That patch added the ovl_is_private_xattr() check in ovl_copy_xattr(),
without which 'error' was always initilalized.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ovl: explicitly initialize error in ovl_copy_xattr()
  2020-06-04 15:57 ` Kees Cook
@ 2020-06-05  9:46   ` Alexander Potapenko
  2020-06-05  9:54   ` Miklos Szeredi
  1 sibling, 0 replies; 10+ messages in thread
From: Alexander Potapenko @ 2020-06-05  9:46 UTC (permalink / raw)
  To: Kees Cook; +Cc: miklos, Vivek Goyal, linux-unionfs, LKML, Roy Yang, stable

On Thu, Jun 4, 2020 at 5:57 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Jun 04, 2020 at 10:42:45AM +0200, glider@google.com wrote:
> > Under certain circumstances (we found this out running Docker on a
> > Clang-built kernel with CONFIG_INIT_STACK_ALL) ovl_copy_xattr() may
> > return uninitialized value of |error| from ovl_copy_xattr().
> > It is then returned by ovl_create() to lookup_open(), which casts it to
> > an invalid dentry pointer, that can be further read or written by the
> > lookup_open() callers.
> >
> > The uninitialized value is returned when all the xattr on the file
> > are ovl_is_private_xattr(), which is actually a successful case,
> > therefore we initialize |error| with 0.
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Roy Yang <royyang@google.com>
> > Cc: <stable@vger.kernel.org> # 4.1
>
> Please include a Fixes (more below) and Link tags for details to help
> guide backporting, then you don't need to bother with with "# 4.1",
> the -stable tools will figure it out with a "Fixes" tag.
>
> Thanks for the v2!
>
> Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1050405
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> > The bug seem to date back to at least v4.1 where the annotation has been
> > introduced (i.e. the compilers started noticing error could be used
> > before being initialized). I hovever didn't try to prove that the
> > problem is actually reproducible on such ancient kernels. We've seen it
> > on a real machine running v4.4 as well.
>
> It seems like it came from this, but that's v4.5:
>
> Fixes: e4ad29fa0d22 ("ovl: use a minimal buffer in ovl_copy_xattr")

https://bugs.chromium.org/p/chromium/issues/detail?id=1050405 mentions v4.4.212.
Your patch could've been slipped into that kernel as well.

> What did you find in v4.1? It looks like error isn't uninitialized in
> v4.1:

The annotation appeared first in
https://elixir.bootlin.com/linux/v4.1.18/source/fs/overlayfs/copy_up.c#L27,
does that count as 4.1 or 4.2?

> But v4.1.52 backported the above patch (e4ad29fa0d22), which is why I
> don't try to figure these things out manually. Once we find the commit,
> the tools will figure it out. I think you just need:
>
> Fixes: e4ad29fa0d22 ("ovl: use a minimal buffer in ovl_copy_xattr")
> Cc: stable@vger.kernel.org

Sounds good!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ovl: explicitly initialize error in ovl_copy_xattr()
  2020-06-04  8:57 ` Miklos Szeredi
@ 2020-06-04 16:00   ` Kees Cook
  0 siblings, 0 replies; 10+ messages in thread
From: Kees Cook @ 2020-06-04 16:00 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: Alexander Potapenko, Vivek Goyal, overlayfs, linux-kernel,
	royyang, stable

On Thu, Jun 04, 2020 at 10:57:24AM +0200, Miklos Szeredi wrote:
> On Thu, Jun 4, 2020 at 10:43 AM <glider@google.com> wrote:
> >
> > Under certain circumstances (we found this out running Docker on a
> > Clang-built kernel with CONFIG_INIT_STACK_ALL) ovl_copy_xattr() may
> > return uninitialized value of |error| from ovl_copy_xattr().
> > It is then returned by ovl_create() to lookup_open(), which casts it to
> > an invalid dentry pointer, that can be further read or written by the
> > lookup_open() callers.
> >
> > The uninitialized value is returned when all the xattr on the file
> > are ovl_is_private_xattr(), which is actually a successful case,
> > therefore we initialize |error| with 0.
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Roy Yang <royyang@google.com>
> > Cc: <stable@vger.kernel.org> # 4.1
> >
> > ---
> >
> > The bug seem to date back to at least v4.1 where the annotation has been
> > introduced (i.e. the compilers started noticing error could be used
> > before being initialized). I hovever didn't try to prove that the
> > problem is actually reproducible on such ancient kernels. We've seen it
> > on a real machine running v4.4 as well.
> >
> > v2:
> >  -- Per Vivek Goyal's suggestion, changed |error| to be 0
> 
> Thanks, applied patch posted here (with your signed-off as well, since
> the patch is the same...):
> 
> https://lore.kernel.org/linux-unionfs/874ks212uj.fsf@m5Zedd9JOGzJrf0/

Can you please add:

Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1050405
Fixes: e4ad29fa0d22 ("ovl: use a minimal buffer in ovl_copy_xattr")
Reviewed-by: Kees Cook <keescook@chromium.org>

(and adjust the CC field to drop the "# 4.1" so tools can figure it
out?)

Thanks!

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ovl: explicitly initialize error in ovl_copy_xattr()
  2020-06-04  8:42 glider
  2020-06-04  8:57 ` Miklos Szeredi
@ 2020-06-04 15:57 ` Kees Cook
  2020-06-05  9:46   ` Alexander Potapenko
  2020-06-05  9:54   ` Miklos Szeredi
  1 sibling, 2 replies; 10+ messages in thread
From: Kees Cook @ 2020-06-04 15:57 UTC (permalink / raw)
  To: glider; +Cc: miklos, vgoyal, linux-unionfs, linux-kernel, royyang, stable

On Thu, Jun 04, 2020 at 10:42:45AM +0200, glider@google.com wrote:
> Under certain circumstances (we found this out running Docker on a
> Clang-built kernel with CONFIG_INIT_STACK_ALL) ovl_copy_xattr() may
> return uninitialized value of |error| from ovl_copy_xattr().
> It is then returned by ovl_create() to lookup_open(), which casts it to
> an invalid dentry pointer, that can be further read or written by the
> lookup_open() callers.
> 
> The uninitialized value is returned when all the xattr on the file
> are ovl_is_private_xattr(), which is actually a successful case,
> therefore we initialize |error| with 0.
> 
> Signed-off-by: Alexander Potapenko <glider@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Roy Yang <royyang@google.com>
> Cc: <stable@vger.kernel.org> # 4.1

Please include a Fixes (more below) and Link tags for details to help
guide backporting, then you don't need to bother with with "# 4.1",
the -stable tools will figure it out with a "Fixes" tag.

Thanks for the v2!

Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1050405
Reviewed-by: Kees Cook <keescook@chromium.org>

> The bug seem to date back to at least v4.1 where the annotation has been
> introduced (i.e. the compilers started noticing error could be used
> before being initialized). I hovever didn't try to prove that the
> problem is actually reproducible on such ancient kernels. We've seen it
> on a real machine running v4.4 as well.

It seems like it came from this, but that's v4.5:

Fixes: e4ad29fa0d22 ("ovl: use a minimal buffer in ovl_copy_xattr")

What did you find in v4.1? It looks like error isn't uninitialized in
v4.1:

int ovl_copy_xattr(struct dentry *old, struct dentry *new)
{
        ssize_t list_size, size;
        char *buf, *name, *value;
        int error;

        if (!old->d_inode->i_op->getxattr ||
            !new->d_inode->i_op->getxattr)
                return 0;

        list_size = vfs_listxattr(old, NULL, 0);
        if (list_size <= 0) {
                if (list_size == -EOPNOTSUPP)
                        return 0;
                return list_size;
        }

        buf = kzalloc(list_size, GFP_KERNEL);
        if (!buf)
                return -ENOMEM;

        error = -ENOMEM;
...

But v4.1.52 backported the above patch (e4ad29fa0d22), which is why I
don't try to figure these things out manually. Once we find the commit,
the tools will figure it out. I think you just need:

Fixes: e4ad29fa0d22 ("ovl: use a minimal buffer in ovl_copy_xattr")
Cc: stable@vger.kernel.org

and things like v4.1.52 will get fixed (if anyone is actually doing
updates for v4.1.z any more...)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] ovl: explicitly initialize error in ovl_copy_xattr()
  2020-06-04  8:42 glider
@ 2020-06-04  8:57 ` Miklos Szeredi
  2020-06-04 16:00   ` Kees Cook
  2020-06-04 15:57 ` Kees Cook
  1 sibling, 1 reply; 10+ messages in thread
From: Miklos Szeredi @ 2020-06-04  8:57 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: Vivek Goyal, overlayfs, linux-kernel, Kees Cook, royyang, stable

On Thu, Jun 4, 2020 at 10:43 AM <glider@google.com> wrote:
>
> Under certain circumstances (we found this out running Docker on a
> Clang-built kernel with CONFIG_INIT_STACK_ALL) ovl_copy_xattr() may
> return uninitialized value of |error| from ovl_copy_xattr().
> It is then returned by ovl_create() to lookup_open(), which casts it to
> an invalid dentry pointer, that can be further read or written by the
> lookup_open() callers.
>
> The uninitialized value is returned when all the xattr on the file
> are ovl_is_private_xattr(), which is actually a successful case,
> therefore we initialize |error| with 0.
>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Roy Yang <royyang@google.com>
> Cc: <stable@vger.kernel.org> # 4.1
>
> ---
>
> The bug seem to date back to at least v4.1 where the annotation has been
> introduced (i.e. the compilers started noticing error could be used
> before being initialized). I hovever didn't try to prove that the
> problem is actually reproducible on such ancient kernels. We've seen it
> on a real machine running v4.4 as well.
>
> v2:
>  -- Per Vivek Goyal's suggestion, changed |error| to be 0

Thanks, applied patch posted here (with your signed-off as well, since
the patch is the same...):

https://lore.kernel.org/linux-unionfs/874ks212uj.fsf@m5Zedd9JOGzJrf0/

Miklos

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] ovl: explicitly initialize error in ovl_copy_xattr()
@ 2020-06-04  8:42 glider
  2020-06-04  8:57 ` Miklos Szeredi
  2020-06-04 15:57 ` Kees Cook
  0 siblings, 2 replies; 10+ messages in thread
From: glider @ 2020-06-04  8:42 UTC (permalink / raw)
  To: miklos, vgoyal
  Cc: linux-unionfs, linux-kernel, keescook, royyang, stable,
	Alexander Potapenko

Under certain circumstances (we found this out running Docker on a
Clang-built kernel with CONFIG_INIT_STACK_ALL) ovl_copy_xattr() may
return uninitialized value of |error| from ovl_copy_xattr().
It is then returned by ovl_create() to lookup_open(), which casts it to
an invalid dentry pointer, that can be further read or written by the
lookup_open() callers.

The uninitialized value is returned when all the xattr on the file
are ovl_is_private_xattr(), which is actually a successful case,
therefore we initialize |error| with 0.

Signed-off-by: Alexander Potapenko <glider@google.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Roy Yang <royyang@google.com>
Cc: <stable@vger.kernel.org> # 4.1

---

The bug seem to date back to at least v4.1 where the annotation has been
introduced (i.e. the compilers started noticing error could be used
before being initialized). I hovever didn't try to prove that the
problem is actually reproducible on such ancient kernels. We've seen it
on a real machine running v4.4 as well.

v2:
 -- Per Vivek Goyal's suggestion, changed |error| to be 0
---
 fs/overlayfs/copy_up.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 9709cf22cab3..07e0d1961e96 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -47,7 +47,7 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
 {
 	ssize_t list_size, size, value_size = 0;
 	char *buf, *name, *value = NULL;
-	int uninitialized_var(error);
+	int error = 0;
 	size_t slen;
 
 	if (!(old->d_inode->i_opflags & IOP_XATTR) ||
-- 
2.27.0.278.ge193c7cf3a9-goog


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-03 17:47 [PATCH] ovl: explicitly initialize error in ovl_copy_xattr() glider
2020-06-03 20:35 ` Kees Cook
2020-06-03 21:46 ` Vivek Goyal
2020-06-04  8:28   ` Alexander Potapenko
2020-06-04  8:42 glider
2020-06-04  8:57 ` Miklos Szeredi
2020-06-04 16:00   ` Kees Cook
2020-06-04 15:57 ` Kees Cook
2020-06-05  9:46   ` Alexander Potapenko
2020-06-05  9:54   ` Miklos Szeredi

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git