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 <[email protected]>
---
I have 13 more similar places to patch... I'm not totally sure I
understand all the issues involved.
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)
--
2.20.1
Dan Carpenter <[email protected]> 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 <[email protected]>
> ---
> 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)
On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote:
> Dan Carpenter <[email protected]> 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 <[email protected]>
> > ---
> > 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.
I know it might sound quite inefficient, but what about making struct
fb_fix_screeninfo __packed?
This doesn't solve other potential similar issues, but for this
particular case it could be a reasonable and simple fix.
-Andrea
Andrea Righi <[email protected]> writes:
> On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote:
>> Dan Carpenter <[email protected]> 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 <[email protected]>
>> > ---
>> > 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.
>
> I know it might sound quite inefficient, but what about making struct
> fb_fix_screeninfo __packed?
>
> This doesn't solve other potential similar issues, but for this
> particular case it could be a reasonable and simple fix.
It is part of the user space ABI. As such you can't move the fields.
Eric
On Wed, Oct 30, 2019 at 02:26:21PM -0500, Eric W. Biederman wrote:
> Andrea Righi <[email protected]> writes:
>
> > On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote:
> >> Dan Carpenter <[email protected]> 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 <[email protected]>
> >> > ---
> >> > 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.
> >
> > I know it might sound quite inefficient, but what about making struct
> > fb_fix_screeninfo __packed?
> >
> > This doesn't solve other potential similar issues, but for this
> > particular case it could be a reasonable and simple fix.
>
> It is part of the user space ABI. As such you can't move the fields.
>
> Eric
Oh, that's right! Then memset() + memcpy() is probably the best option,
since copying all those fields one by one looks quite ugly to me...
-Andrea
On Wed, 2019-10-30 at 21:12 +0100, Andrea Righi wrote:
> Then memset() + memcpy() is probably the best option,
> since copying all those fields one by one looks quite ugly to me...
A memset of an automatic before a memcpy to the same
automatic is unnecessary.
Joe Perches <[email protected]> writes:
> On Wed, 2019-10-30 at 21:12 +0100, Andrea Righi wrote:
>> Then memset() + memcpy() is probably the best option,
>> since copying all those fields one by one looks quite ugly to me...
>
> A memset of an automatic before a memcpy to the same
> automatic is unnecessary.
You still need to guarantee that all of the holes in the
structure you are copying are initialized before you copy it.
Otherwise you are just changing which unitialized memory that
is being copied to userspace.
Which is my concern with your very simple suggestion.
Eric
On 10/29/19 8:02 PM, Eric W. Biederman wrote:
> Dan Carpenter <[email protected]> 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 <[email protected]>
>> ---
>> 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
"info" (and thus "info->fix") comes from framebuffer_alloc() (which is
called by fbdev device drivers prior to registering "info" with
register_framebuffer()). framebuffer_alloc() does kzalloc() on "info".
Therefore shouldn't memcpy() (as suggested by Jeo Perches) be enough?
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> 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)
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. The solution is just to replace the assignment with an
memcpy().
Fixes: 1f5e31d7e55a ("fbmem: don't call copy_from/to_user() with mutex held")
Signed-off-by: Dan Carpenter <[email protected]>
---
v2: Use memcpy()
drivers/video/fbdev/core/fbmem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index d04554959ea7..bb8d8dbc0461 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1115,7 +1115,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
break;
case FBIOGET_FSCREENINFO:
lock_fb_info(info);
- fix = info->fix;
+ memcpy(&fix, &info->fix, sizeof(fix));
if (info->flags & FBINFO_HIDE_SMEM_START)
fix.smem_start = 0;
unlock_fb_info(info);
--
2.11.0
On Fri, Jan 3, 2020 at 2:09 PM Bartlomiej Zolnierkiewicz
<[email protected]> wrote:
> On 10/29/19 8:02 PM, Eric W. Biederman wrote:
> >
> > 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
>
> "info" (and thus "info->fix") comes from framebuffer_alloc() (which is
> called by fbdev device drivers prior to registering "info" with
> register_framebuffer()). framebuffer_alloc() does kzalloc() on "info".
>
> Therefore shouldn't memcpy() (as suggested by Jeo Perches) be enough?
Is it guaranteed that all drivers call framebuffer_alloc() rather than
open-coding it somewhere?
Here is a list of all files that call register_framebuffer() without first
calling framebuffer_alloc:
$ git grep -wl register_framebuffer | xargs grep -L framebuffer_alloc
Documentation/fb/framebuffer.rst
drivers/media/pci/ivtv/ivtvfb.c
drivers/media/platform/vivid/vivid-osd.c
drivers/video/fbdev/68328fb.c
drivers/video/fbdev/acornfb.c
drivers/video/fbdev/amba-clcd.c
drivers/video/fbdev/atafb.c
drivers/video/fbdev/au1100fb.c
drivers/video/fbdev/controlfb.c
drivers/video/fbdev/core/fbmem.c
drivers/video/fbdev/cyber2000fb.c
drivers/video/fbdev/fsl-diu-fb.c
drivers/video/fbdev/g364fb.c
drivers/video/fbdev/goldfishfb.c
drivers/video/fbdev/hpfb.c
drivers/video/fbdev/macfb.c
drivers/video/fbdev/matrox/matroxfb_base.c
drivers/video/fbdev/matrox/matroxfb_crtc2.c
drivers/video/fbdev/maxinefb.c
drivers/video/fbdev/ocfb.c
drivers/video/fbdev/pxafb.c
drivers/video/fbdev/sa1100fb.c
drivers/video/fbdev/stifb.c
drivers/video/fbdev/valkyriefb.c
drivers/video/fbdev/vermilion/vermilion.c
drivers/video/fbdev/vt8500lcdfb.c
drivers/video/fbdev/wm8505fb.c
drivers/video/fbdev/xilinxfb.c
It's possible (even likely, the ones I looked at are fine) that they
all correctly
zero out the fb_info structure first, but it seems hard to guarantee, so
Eric's suggestion would possibly still be the safer choice.
Arnd
On 1/13/20 1:49 PM, Arnd Bergmann wrote:
> On Fri, Jan 3, 2020 at 2:09 PM Bartlomiej Zolnierkiewicz
> <[email protected]> wrote:
>> On 10/29/19 8:02 PM, Eric W. Biederman wrote:
>>>
>>> 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
>>
>> "info" (and thus "info->fix") comes from framebuffer_alloc() (which is
>> called by fbdev device drivers prior to registering "info" with
>> register_framebuffer()). framebuffer_alloc() does kzalloc() on "info".
>>
>> Therefore shouldn't memcpy() (as suggested by Jeo Perches) be enough?
>
> Is it guaranteed that all drivers call framebuffer_alloc() rather than
> open-coding it somewhere?
>
> Here is a list of all files that call register_framebuffer() without first
> calling framebuffer_alloc:
>
> $ git grep -wl register_framebuffer | xargs grep -L framebuffer_alloc
> Documentation/fb/framebuffer.rst
> drivers/media/pci/ivtv/ivtvfb.c
> drivers/media/platform/vivid/vivid-osd.c
> drivers/video/fbdev/68328fb.c
> drivers/video/fbdev/acornfb.c
> drivers/video/fbdev/amba-clcd.c
> drivers/video/fbdev/atafb.c
> drivers/video/fbdev/au1100fb.c
> drivers/video/fbdev/controlfb.c
> drivers/video/fbdev/core/fbmem.c
> drivers/video/fbdev/cyber2000fb.c
> drivers/video/fbdev/fsl-diu-fb.c
> drivers/video/fbdev/g364fb.c
> drivers/video/fbdev/goldfishfb.c
> drivers/video/fbdev/hpfb.c
> drivers/video/fbdev/macfb.c
> drivers/video/fbdev/matrox/matroxfb_base.c
> drivers/video/fbdev/matrox/matroxfb_crtc2.c
> drivers/video/fbdev/maxinefb.c
> drivers/video/fbdev/ocfb.c
> drivers/video/fbdev/pxafb.c
> drivers/video/fbdev/sa1100fb.c
> drivers/video/fbdev/stifb.c
> drivers/video/fbdev/valkyriefb.c
> drivers/video/fbdev/vermilion/vermilion.c
> drivers/video/fbdev/vt8500lcdfb.c
> drivers/video/fbdev/wm8505fb.c
> drivers/video/fbdev/xilinxfb.c
>
> It's possible (even likely, the ones I looked at are fine) that they
> all correctly
> zero out the fb_info structure first, but it seems hard to guarantee, so
> Eric's suggestion would possibly still be the safer choice.
I've audited all above instances and they are all fine. They either
use the fb_info structure embedded in a driver specific structure
(which is zeroed out) or (in case of some m68k specific drivers) use
a static fb_info instance.
Since fbdev is closed for new drivers it should be now fine to use
the simpler approach (just use memcpy()).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
On Wed, Jan 15, 2020 at 2:09 PM Bartlomiej Zolnierkiewicz
<[email protected]> wrote:
> > $ git grep -wl register_framebuffer | xargs grep -L framebuffer_alloc
> > Documentation/fb/framebuffer.rst
> > drivers/media/pci/ivtv/ivtvfb.c
> > drivers/media/platform/vivid/vivid-osd.c
> > drivers/video/fbdev/68328fb.c
> > drivers/video/fbdev/acornfb.c
> > drivers/video/fbdev/amba-clcd.c
> > drivers/video/fbdev/atafb.c
> > drivers/video/fbdev/au1100fb.c
> > drivers/video/fbdev/controlfb.c
> > drivers/video/fbdev/core/fbmem.c
> > drivers/video/fbdev/cyber2000fb.c
> > drivers/video/fbdev/fsl-diu-fb.c
> > drivers/video/fbdev/g364fb.c
> > drivers/video/fbdev/goldfishfb.c
> > drivers/video/fbdev/hpfb.c
> > drivers/video/fbdev/macfb.c
> > drivers/video/fbdev/matrox/matroxfb_base.c
> > drivers/video/fbdev/matrox/matroxfb_crtc2.c
> > drivers/video/fbdev/maxinefb.c
> > drivers/video/fbdev/ocfb.c
> > drivers/video/fbdev/pxafb.c
> > drivers/video/fbdev/sa1100fb.c
> > drivers/video/fbdev/stifb.c
> > drivers/video/fbdev/valkyriefb.c
> > drivers/video/fbdev/vermilion/vermilion.c
> > drivers/video/fbdev/vt8500lcdfb.c
> > drivers/video/fbdev/wm8505fb.c
> > drivers/video/fbdev/xilinxfb.c
> >
> > It's possible (even likely, the ones I looked at are fine) that they
> > all correctly
> > zero out the fb_info structure first, but it seems hard to guarantee, so
> > Eric's suggestion would possibly still be the safer choice.
>
> I've audited all above instances and they are all fine. They either
> use the fb_info structure embedded in a driver specific structure
> (which is zeroed out) or (in case of some m68k specific drivers) use
> a static fb_info instance.
>
> Since fbdev is closed for new drivers it should be now fine to use
> the simpler approach (just use memcpy()).
Yes, let's do that then. The complex approach seems more likely
to introduce a bug than one of the existing drivers to stop initializing
the structure correctly.
Arnd
On 1/13/20 12:08 PM, Dan Carpenter wrote:
> 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. The solution is just to replace the assignment with an
> memcpy().
>
> Fixes: 1f5e31d7e55a ("fbmem: don't call copy_from/to_user() with mutex held")
> Signed-off-by: Dan Carpenter <[email protected]>
Patch queued for v5.6, thanks.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> ---
> v2: Use memcpy()
>
> drivers/video/fbdev/core/fbmem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index d04554959ea7..bb8d8dbc0461 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1115,7 +1115,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> break;
> case FBIOGET_FSCREENINFO:
> lock_fb_info(info);
> - fix = info->fix;
> + memcpy(&fix, &info->fix, sizeof(fix));
> if (info->flags & FBINFO_HIDE_SMEM_START)
> fix.smem_start = 0;
> unlock_fb_info(info);
>