* [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-29 18:23 ` Dan Carpenter 0 siblings, 0 replies; 42+ messages in thread From: Dan Carpenter @ 2019-10-29 18:23 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz, Andrea Righi Cc: Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall 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@oracle.com> --- 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-29 18:23 ` Dan Carpenter 0 siblings, 0 replies; 42+ messages in thread From: Dan Carpenter @ 2019-10-29 18:23 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz, Andrea Righi Cc: linux-fbdev, security, Kees Cook, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Gerd Hoffmann, Sam Ravnborg, Peter Rosin 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@oracle.com> --- 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-29 18:23 ` Dan Carpenter 0 siblings, 0 replies; 42+ messages in thread From: Dan Carpenter @ 2019-10-29 18:23 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz, Andrea Righi Cc: linux-fbdev, security, Kees Cook, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Gerd Hoffmann, Sam Ravnborg, Peter Rosin 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@oracle.com> --- 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() 2019-10-29 18:23 ` Dan Carpenter (?) @ 2019-10-29 18:35 ` Joe Perches -1 siblings, 0 replies; 42+ messages in thread From: Joe Perches @ 2019-10-29 18:35 UTC (permalink / raw) To: Dan Carpenter, Bartlomiej Zolnierkiewicz, Andrea Righi Cc: Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall On Tue, 2019-10-29 at 21:23 +0300, 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. [] > diff --git 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) Perhaps better to change the struct copy to a 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 e6a1c80..364699 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1110,7 +1110,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); ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-29 18:35 ` Joe Perches 0 siblings, 0 replies; 42+ messages in thread From: Joe Perches @ 2019-10-29 18:35 UTC (permalink / raw) To: Dan Carpenter, Bartlomiej Zolnierkiewicz, Andrea Righi Cc: linux-fbdev, security, Kees Cook, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Gerd Hoffmann, Sam Ravnborg, Peter Rosin On Tue, 2019-10-29 at 21:23 +0300, 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. [] > diff --git 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) Perhaps better to change the struct copy to a 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 e6a1c80..364699 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1110,7 +1110,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); _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-29 18:35 ` Joe Perches 0 siblings, 0 replies; 42+ messages in thread From: Joe Perches @ 2019-10-29 18:35 UTC (permalink / raw) To: Dan Carpenter, Bartlomiej Zolnierkiewicz, Andrea Righi Cc: Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall On Tue, 2019-10-29 at 21:23 +0300, 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. [] > diff --git 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) Perhaps better to change the struct copy to a 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 e6a1c80..364699 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1110,7 +1110,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); ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() 2019-10-29 18:23 ` Dan Carpenter (?) @ 2019-10-29 19:02 ` Eric W. Biederman -1 siblings, 0 replies; 42+ messages in thread From: Eric W. Biederman @ 2019-10-29 19:02 UTC (permalink / raw) To: Dan Carpenter Cc: Bartlomiej Zolnierkiewicz, Andrea Righi, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall Dan Carpenter <dan.carpenter@oracle.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@oracle.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) ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-29 19:02 ` Eric W. Biederman 0 siblings, 0 replies; 42+ messages in thread From: Eric W. Biederman @ 2019-10-29 19:02 UTC (permalink / raw) To: Dan Carpenter Cc: linux-fbdev, security, Kees Cook, Bartlomiej Zolnierkiewicz, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Gerd Hoffmann, Sam Ravnborg, Peter Rosin, Andrea Righi Dan Carpenter <dan.carpenter@oracle.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@oracle.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) _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-29 19:02 ` Eric W. Biederman 0 siblings, 0 replies; 42+ messages in thread From: Eric W. Biederman @ 2019-10-29 19:02 UTC (permalink / raw) To: Dan Carpenter Cc: Bartlomiej Zolnierkiewicz, Andrea Righi, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall Dan Carpenter <dan.carpenter@oracle.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@oracle.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) ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() 2019-10-29 19:02 ` Eric W. Biederman (?) @ 2019-10-30 7:43 ` Andrea Righi -1 siblings, 0 replies; 42+ messages in thread From: Andrea Righi @ 2019-10-30 7:43 UTC (permalink / raw) To: Eric W. Biederman Cc: Dan Carpenter, Bartlomiej Zolnierkiewicz, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote: > Dan Carpenter <dan.carpenter@oracle.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@oracle.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. 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-30 7:43 ` Andrea Righi 0 siblings, 0 replies; 42+ messages in thread From: Andrea Righi @ 2019-10-30 7:43 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-fbdev, security, Kees Cook, Bartlomiej Zolnierkiewicz, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Gerd Hoffmann, Sam Ravnborg, Peter Rosin, Dan Carpenter On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote: > Dan Carpenter <dan.carpenter@oracle.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@oracle.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. 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-30 7:43 ` Andrea Righi 0 siblings, 0 replies; 42+ messages in thread From: Andrea Righi @ 2019-10-30 7:43 UTC (permalink / raw) To: Eric W. Biederman Cc: Dan Carpenter, Bartlomiej Zolnierkiewicz, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote: > Dan Carpenter <dan.carpenter@oracle.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@oracle.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. 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() 2019-10-30 7:43 ` Andrea Righi (?) @ 2019-10-30 19:26 ` Eric W. Biederman -1 siblings, 0 replies; 42+ messages in thread From: Eric W. Biederman @ 2019-10-30 19:26 UTC (permalink / raw) To: Andrea Righi Cc: Dan Carpenter, Bartlomiej Zolnierkiewicz, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall Andrea Righi <andrea.righi@canonical.com> writes: > On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote: >> Dan Carpenter <dan.carpenter@oracle.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@oracle.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. > > 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-30 19:26 ` Eric W. Biederman 0 siblings, 0 replies; 42+ messages in thread From: Eric W. Biederman @ 2019-10-30 19:26 UTC (permalink / raw) To: Andrea Righi Cc: linux-fbdev, security, Kees Cook, Bartlomiej Zolnierkiewicz, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Gerd Hoffmann, Sam Ravnborg, Peter Rosin, Dan Carpenter Andrea Righi <andrea.righi@canonical.com> writes: > On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote: >> Dan Carpenter <dan.carpenter@oracle.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@oracle.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. > > 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-30 19:26 ` Eric W. Biederman 0 siblings, 0 replies; 42+ messages in thread From: Eric W. Biederman @ 2019-10-30 19:26 UTC (permalink / raw) To: Andrea Righi Cc: Dan Carpenter, Bartlomiej Zolnierkiewicz, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall Andrea Righi <andrea.righi@canonical.com> writes: > On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote: >> Dan Carpenter <dan.carpenter@oracle.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@oracle.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. > > 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() 2019-10-30 19:26 ` Eric W. Biederman (?) @ 2019-10-30 20:12 ` Andrea Righi -1 siblings, 0 replies; 42+ messages in thread From: Andrea Righi @ 2019-10-30 20:12 UTC (permalink / raw) To: Eric W. Biederman Cc: Dan Carpenter, Bartlomiej Zolnierkiewicz, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall On Wed, Oct 30, 2019 at 02:26:21PM -0500, Eric W. Biederman wrote: > Andrea Righi <andrea.righi@canonical.com> writes: > > > On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote: > >> Dan Carpenter <dan.carpenter@oracle.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@oracle.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. > > > > 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-30 20:12 ` Andrea Righi 0 siblings, 0 replies; 42+ messages in thread From: Andrea Righi @ 2019-10-30 20:12 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-fbdev, security, Kees Cook, Bartlomiej Zolnierkiewicz, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Gerd Hoffmann, Sam Ravnborg, Peter Rosin, Dan Carpenter On Wed, Oct 30, 2019 at 02:26:21PM -0500, Eric W. Biederman wrote: > Andrea Righi <andrea.righi@canonical.com> writes: > > > On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote: > >> Dan Carpenter <dan.carpenter@oracle.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@oracle.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. > > > > 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-30 20:12 ` Andrea Righi 0 siblings, 0 replies; 42+ messages in thread From: Andrea Righi @ 2019-10-30 20:12 UTC (permalink / raw) To: Eric W. Biederman Cc: Dan Carpenter, Bartlomiej Zolnierkiewicz, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall On Wed, Oct 30, 2019 at 02:26:21PM -0500, Eric W. Biederman wrote: > Andrea Righi <andrea.righi@canonical.com> writes: > > > On Tue, Oct 29, 2019 at 02:02:11PM -0500, Eric W. Biederman wrote: > >> Dan Carpenter <dan.carpenter@oracle.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@oracle.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. > > > > 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() 2019-10-30 20:12 ` Andrea Righi (?) @ 2019-10-31 18:16 ` Joe Perches -1 siblings, 0 replies; 42+ messages in thread From: Joe Perches @ 2019-10-31 18:16 UTC (permalink / raw) To: Andrea Righi, Eric W. Biederman Cc: Dan Carpenter, Bartlomiej Zolnierkiewicz, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall 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. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-31 18:16 ` Joe Perches 0 siblings, 0 replies; 42+ messages in thread From: Joe Perches @ 2019-10-31 18:16 UTC (permalink / raw) To: Andrea Righi, Eric W. Biederman Cc: linux-fbdev, security, Kees Cook, Bartlomiej Zolnierkiewicz, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Gerd Hoffmann, Sam Ravnborg, Peter Rosin, Dan Carpenter 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. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-31 18:16 ` Joe Perches 0 siblings, 0 replies; 42+ messages in thread From: Joe Perches @ 2019-10-31 18:16 UTC (permalink / raw) To: Andrea Righi, Eric W. Biederman Cc: Dan Carpenter, Bartlomiej Zolnierkiewicz, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall 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. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() 2019-10-31 18:16 ` Joe Perches (?) @ 2019-10-31 22:12 ` Eric W. Biederman -1 siblings, 0 replies; 42+ messages in thread From: Eric W. Biederman @ 2019-10-31 22:12 UTC (permalink / raw) To: Joe Perches Cc: Andrea Righi, Dan Carpenter, Bartlomiej Zolnierkiewicz, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall Joe Perches <joe@perches.com> 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-31 22:12 ` Eric W. Biederman 0 siblings, 0 replies; 42+ messages in thread From: Eric W. Biederman @ 2019-10-31 22:12 UTC (permalink / raw) To: Joe Perches Cc: linux-fbdev, security, Kees Cook, Julia Lawall, Bartlomiej Zolnierkiewicz, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Andrea Righi, Gerd Hoffmann, Sam Ravnborg, Peter Rosin, Dan Carpenter Joe Perches <joe@perches.com> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2019-10-31 22:12 ` Eric W. Biederman 0 siblings, 0 replies; 42+ messages in thread From: Eric W. Biederman @ 2019-10-31 22:12 UTC (permalink / raw) To: Joe Perches Cc: Andrea Righi, Dan Carpenter, Bartlomiej Zolnierkiewicz, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall Joe Perches <joe@perches.com> 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() 2019-10-29 19:02 ` Eric W. Biederman (?) @ 2020-01-03 13:07 ` Bartlomiej Zolnierkiewicz -1 siblings, 0 replies; 42+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-01-03 13:07 UTC (permalink / raw) To: Eric W. Biederman, Joe Perches Cc: Dan Carpenter, Andrea Righi, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall On 10/29/19 8:02 PM, Eric W. Biederman wrote: > Dan Carpenter <dan.carpenter@oracle.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@oracle.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 "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) ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2020-01-03 13:07 ` Bartlomiej Zolnierkiewicz 0 siblings, 0 replies; 42+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-01-03 13:07 UTC (permalink / raw) To: Eric W. Biederman, Joe Perches Cc: linux-fbdev, security, Kees Cook, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Gerd Hoffmann, Sam Ravnborg, Peter Rosin, Dan Carpenter, Andrea Righi On 10/29/19 8:02 PM, Eric W. Biederman wrote: > Dan Carpenter <dan.carpenter@oracle.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@oracle.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 "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) _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2020-01-03 13:07 ` Bartlomiej Zolnierkiewicz 0 siblings, 0 replies; 42+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-01-03 13:07 UTC (permalink / raw) To: Eric W. Biederman, Joe Perches Cc: linux-fbdev, security, Kees Cook, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Gerd Hoffmann, Sam Ravnborg, Peter Rosin, Dan Carpenter, Andrea Righi On 10/29/19 8:02 PM, Eric W. Biederman wrote: > Dan Carpenter <dan.carpenter@oracle.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@oracle.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 "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) ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2] fbdev: potential information leak in do_fb_ioctl() 2020-01-03 13:07 ` Bartlomiej Zolnierkiewicz (?) @ 2020-01-13 11:08 ` Dan Carpenter -1 siblings, 0 replies; 42+ messages in thread From: Dan Carpenter @ 2020-01-13 11:08 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Andrew Morton, Arnd Bergmann, Eric W. Biederman, Andrea Righi, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Daniel Thompson, Peter Rosin, Jani Nikula, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors 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 <dan.carpenter@oracle.com> --- 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2] fbdev: potential information leak in do_fb_ioctl() @ 2020-01-13 11:08 ` Dan Carpenter 0 siblings, 0 replies; 42+ messages in thread From: Dan Carpenter @ 2020-01-13 11:08 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Daniel Thompson, Gerd Hoffmann, Arnd Bergmann, Jani Nikula, Daniel Vetter, linux-fbdev, dri-devel, linux-kernel, kernel-janitors, Eric W. Biederman, Andrew Morton, Sam Ravnborg, Peter Rosin, Andrea Righi 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 <dan.carpenter@oracle.com> --- 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2] fbdev: potential information leak in do_fb_ioctl() @ 2020-01-13 11:08 ` Dan Carpenter 0 siblings, 0 replies; 42+ messages in thread From: Dan Carpenter @ 2020-01-13 11:08 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Daniel Thompson, Gerd Hoffmann, Arnd Bergmann, Jani Nikula, Daniel Vetter, linux-fbdev, dri-devel, linux-kernel, kernel-janitors, Eric W. Biederman, Andrew Morton, Sam Ravnborg, Peter Rosin, Andrea Righi 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 <dan.carpenter@oracle.com> --- 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2] fbdev: potential information leak in do_fb_ioctl() 2020-01-13 11:08 ` Dan Carpenter (?) @ 2020-01-15 14:31 ` Bartlomiej Zolnierkiewicz -1 siblings, 0 replies; 42+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-01-15 14:31 UTC (permalink / raw) To: Dan Carpenter Cc: Andrew Morton, Arnd Bergmann, Eric W. Biederman, Andrea Righi, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Daniel Thompson, Peter Rosin, Jani Nikula, Gerd Hoffmann, dri-devel, linux-fbdev, linux-kernel, kernel-janitors 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 <dan.carpenter@oracle.com> 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); > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] fbdev: potential information leak in do_fb_ioctl() @ 2020-01-15 14:31 ` Bartlomiej Zolnierkiewicz 0 siblings, 0 replies; 42+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-01-15 14:31 UTC (permalink / raw) To: Dan Carpenter Cc: Daniel Thompson, Gerd Hoffmann, Arnd Bergmann, Jani Nikula, Daniel Vetter, linux-fbdev, dri-devel, linux-kernel, kernel-janitors, Eric W. Biederman, Andrew Morton, Sam Ravnborg, Peter Rosin, Andrea Righi 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 <dan.carpenter@oracle.com> 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); > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2] fbdev: potential information leak in do_fb_ioctl() @ 2020-01-15 14:31 ` Bartlomiej Zolnierkiewicz 0 siblings, 0 replies; 42+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-01-15 14:31 UTC (permalink / raw) To: Dan Carpenter Cc: Daniel Thompson, Gerd Hoffmann, Arnd Bergmann, Jani Nikula, Daniel Vetter, linux-fbdev, dri-devel, linux-kernel, kernel-janitors, Eric W. Biederman, Andrew Morton, Sam Ravnborg, Peter Rosin, Andrea Righi 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 <dan.carpenter@oracle.com> 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); > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() 2020-01-03 13:07 ` Bartlomiej Zolnierkiewicz (?) @ 2020-01-13 12:49 ` Arnd Bergmann -1 siblings, 0 replies; 42+ messages in thread From: Arnd Bergmann @ 2020-01-13 12:49 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Eric W. Biederman, Joe Perches, Dan Carpenter, Andrea Righi, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, Linux Fbdev development list, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall On Fri, Jan 3, 2020 at 2:09 PM Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2020-01-13 12:49 ` Arnd Bergmann 0 siblings, 0 replies; 42+ messages in thread From: Arnd Bergmann @ 2020-01-13 12:49 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Linux Fbdev development list, security, Gerd Hoffmann, Kees Cook, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Eric W. Biederman, Joe Perches, Sam Ravnborg, Peter Rosin, Dan Carpenter, Andrea Righi On Fri, Jan 3, 2020 at 2:09 PM Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2020-01-13 12:49 ` Arnd Bergmann 0 siblings, 0 replies; 42+ messages in thread From: Arnd Bergmann @ 2020-01-13 12:49 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Linux Fbdev development list, security, Gerd Hoffmann, Kees Cook, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Eric W. Biederman, Joe Perches, Sam Ravnborg, Peter Rosin, Dan Carpenter, Andrea Righi On Fri, Jan 3, 2020 at 2:09 PM Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() 2020-01-13 12:49 ` Arnd Bergmann (?) @ 2020-01-15 13:09 ` Bartlomiej Zolnierkiewicz -1 siblings, 0 replies; 42+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-01-15 13:09 UTC (permalink / raw) To: Arnd Bergmann Cc: Eric W. Biederman, Joe Perches, Dan Carpenter, Andrea Righi, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, Linux Fbdev development list, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall On 1/13/20 1:49 PM, Arnd Bergmann wrote: > On Fri, Jan 3, 2020 at 2:09 PM Bartlomiej Zolnierkiewicz > <b.zolnierkie@samsung.com> 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2020-01-15 13:09 ` Bartlomiej Zolnierkiewicz 0 siblings, 0 replies; 42+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-01-15 13:09 UTC (permalink / raw) To: Arnd Bergmann Cc: Linux Fbdev development list, security, Gerd Hoffmann, Kees Cook, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Eric W. Biederman, Joe Perches, Sam Ravnborg, Peter Rosin, Dan Carpenter, Andrea Righi On 1/13/20 1:49 PM, Arnd Bergmann wrote: > On Fri, Jan 3, 2020 at 2:09 PM Bartlomiej Zolnierkiewicz > <b.zolnierkie@samsung.com> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2020-01-15 13:09 ` Bartlomiej Zolnierkiewicz 0 siblings, 0 replies; 42+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2020-01-15 13:09 UTC (permalink / raw) To: Arnd Bergmann Cc: Linux Fbdev development list, security, Gerd Hoffmann, Kees Cook, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Eric W. Biederman, Joe Perches, Sam Ravnborg, Peter Rosin, Dan Carpenter, Andrea Righi On 1/13/20 1:49 PM, Arnd Bergmann wrote: > On Fri, Jan 3, 2020 at 2:09 PM Bartlomiej Zolnierkiewicz > <b.zolnierkie@samsung.com> 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() 2020-01-15 13:09 ` Bartlomiej Zolnierkiewicz (?) @ 2020-01-15 13:16 ` Arnd Bergmann -1 siblings, 0 replies; 42+ messages in thread From: Arnd Bergmann @ 2020-01-15 13:16 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Eric W. Biederman, Joe Perches, Dan Carpenter, Andrea Righi, Daniel Vetter, Sam Ravnborg, Maarten Lankhorst, Peter Rosin, Gerd Hoffmann, dri-devel, Linux Fbdev development list, linux-kernel, kernel-janitors, security, Kees Cook, Julia Lawall On Wed, Jan 15, 2020 at 2:09 PM Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2020-01-15 13:16 ` Arnd Bergmann 0 siblings, 0 replies; 42+ messages in thread From: Arnd Bergmann @ 2020-01-15 13:16 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Linux Fbdev development list, security, Gerd Hoffmann, Kees Cook, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Eric W. Biederman, Joe Perches, Sam Ravnborg, Peter Rosin, Dan Carpenter, Andrea Righi On Wed, Jan 15, 2020 at 2:09 PM Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] fbdev: potential information leak in do_fb_ioctl() @ 2020-01-15 13:16 ` Arnd Bergmann 0 siblings, 0 replies; 42+ messages in thread From: Arnd Bergmann @ 2020-01-15 13:16 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: Linux Fbdev development list, security, Gerd Hoffmann, Kees Cook, kernel-janitors, Daniel Vetter, linux-kernel, dri-devel, Julia Lawall, Eric W. Biederman, Joe Perches, Sam Ravnborg, Peter Rosin, Dan Carpenter, Andrea Righi On Wed, Jan 15, 2020 at 2:09 PM Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2020-01-15 14:31 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-29 18:23 [PATCH] fbdev: potential information leak in do_fb_ioctl() Dan Carpenter 2019-10-29 18:23 ` Dan Carpenter 2019-10-29 18:23 ` Dan Carpenter 2019-10-29 18:35 ` Joe Perches 2019-10-29 18:35 ` Joe Perches 2019-10-29 18:35 ` Joe Perches 2019-10-29 19:02 ` Eric W. Biederman 2019-10-29 19:02 ` Eric W. Biederman 2019-10-29 19:02 ` Eric W. Biederman 2019-10-30 7:43 ` Andrea Righi 2019-10-30 7:43 ` Andrea Righi 2019-10-30 7:43 ` Andrea Righi 2019-10-30 19:26 ` Eric W. Biederman 2019-10-30 19:26 ` Eric W. Biederman 2019-10-30 19:26 ` Eric W. Biederman 2019-10-30 20:12 ` Andrea Righi 2019-10-30 20:12 ` Andrea Righi 2019-10-30 20:12 ` Andrea Righi 2019-10-31 18:16 ` Joe Perches 2019-10-31 18:16 ` Joe Perches 2019-10-31 18:16 ` Joe Perches 2019-10-31 22:12 ` Eric W. Biederman 2019-10-31 22:12 ` Eric W. Biederman 2019-10-31 22:12 ` Eric W. Biederman 2020-01-03 13:07 ` Bartlomiej Zolnierkiewicz 2020-01-03 13:07 ` Bartlomiej Zolnierkiewicz 2020-01-03 13:07 ` Bartlomiej Zolnierkiewicz 2020-01-13 11:08 ` [PATCH v2] " Dan Carpenter 2020-01-13 11:08 ` Dan Carpenter 2020-01-13 11:08 ` Dan Carpenter 2020-01-15 14:31 ` Bartlomiej Zolnierkiewicz 2020-01-15 14:31 ` Bartlomiej Zolnierkiewicz 2020-01-15 14:31 ` Bartlomiej Zolnierkiewicz 2020-01-13 12:49 ` [PATCH] " Arnd Bergmann 2020-01-13 12:49 ` Arnd Bergmann 2020-01-13 12:49 ` Arnd Bergmann 2020-01-15 13:09 ` Bartlomiej Zolnierkiewicz 2020-01-15 13:09 ` Bartlomiej Zolnierkiewicz 2020-01-15 13:09 ` Bartlomiej Zolnierkiewicz 2020-01-15 13:16 ` Arnd Bergmann 2020-01-15 13:16 ` Arnd Bergmann 2020-01-15 13:16 ` Arnd Bergmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.