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] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 29 Oct 2019 14:02:11 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Andrea Righi <righi.andrea@...il.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Sam Ravnborg <sam@...nborg.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Peter Rosin <peda@...ntia.se>,
        Gerd Hoffmann <kraxel@...hat.com>,
        dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        security@...nel.org, Kees Cook <keescook@...omium.org>,
        Julia Lawall <Julia.Lawall@...6.fr>
Subject: Re: [PATCH] fbdev: potential information leak in do_fb_ioctl()

Dan Carpenter <dan.carpenter@...cle.com> writes:

> The "fix" struct has a 2 byte hole after ->ywrapstep and the
> "fix = info->fix;" assignment doesn't necessarily clear it.  It depends
> on the compiler.
>
> Fixes: 1f5e31d7e55a ("fbmem: don't call copy_from/to_user() with mutex held")
> Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
> ---
> I have 13 more similar places to patch...  I'm not totally sure I
> understand all the issues involved.

What I have done in a similar situation with struct siginfo, is that
where the structure first appears I have initialized it with memset,
and then field by field.

Then when the structure is copied I copy the structure with memcpy.

That ensures all of the bytes in the original structure are initialized
and that all of the bytes are copied.

The goal is to avoid memory that has values of the previous users of
that memory region from leaking to userspace.  Which depending on who
the previous user of that memory region is could tell userspace
information about what the kernel is doing that it should not be allowed
to find out.

I tried to trace through where "info" and thus presumably "info->fix" is
coming from and only made it as far as  register_framebuffer.  Given
that I suspect a local memset, and then a field by field copy right
before copy_to_user might be a sound solution.  But ick.  That is a lot
of fields to copy.


Eric



>  drivers/video/fbdev/core/fbmem.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 6f6fc785b545..b4ce6a28aed9 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1109,6 +1109,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>  			ret = -EFAULT;
>  		break;
>  	case FBIOGET_FSCREENINFO:
> +		memset(&fix, 0, sizeof(fix));
>  		lock_fb_info(info);
>  		fix = info->fix;
>  		if (info->flags & FBINFO_HIDE_SMEM_START)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ