| Message ID | 20221230063528.41037-1-zh.nvgt@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | fbmem: prevent potential use-after-free issues with console_lock() | expand |
On 12/30/22 07:35, Hang Zhang wrote: > In do_fb_ioctl(), user specified "fb_info" can be freed in the callee > fbcon_get_con2fb_map_ioctl() -> set_con2fb_map() -> > con2fb_release_oldinfo(), this free operation is protected by > console_lock() in fbcon_set_con2fb_map_ioctl(), it also results in > the change of certain states such as "minfo->dead" in matroxfb_remove(), > so that it can be checked to avoid use-after-free before the use sites > (e.g., the check at the beginning of matroxfb_ioctl()). However, > the problem is that the use site is not protected by the same locks > as for the free operation, e.g., "default" case in do_fb_ioctl() > can lead to "matroxfb_ioctl()" but it's not protected by console_lock(), > which can invalidate the aforementioned state set and check in a > concurrent setting. > > Prevent the potential use-after-free issues by protecting the "default" > case in do_fb_ioctl() with console_lock(), similarly as for many other > cases like "case FBIOBLANK" and "case FBIOPAN_DISPLAY". > > Signed-off-by: Hang Zhang <zh.nvgt@gmail.com> applied to fbdev git tree. Thanks, Helge > --- > drivers/video/fbdev/core/fbmem.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 1e70d8c67653..8b1a1527d18a 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1182,6 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > console_unlock(); > break; > default: > + console_lock(); > lock_fb_info(info); > fb = info->fbops; > if (fb->fb_ioctl) > @@ -1189,6 +1190,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > else > ret = -ENOTTY; > unlock_fb_info(info); > + console_unlock(); > } > return ret; > }
On Thu, Jan 5, 2023 at 5:25 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, 5 Jan 2023 at 11:21, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > Hi Helge > > > > On Mon, 2 Jan 2023 at 16:28, Helge Deller <deller@gmx.de> wrote: > > > > > > On 12/30/22 07:35, Hang Zhang wrote: > > > > In do_fb_ioctl(), user specified "fb_info" can be freed in the callee > > > > fbcon_get_con2fb_map_ioctl() -> set_con2fb_map() -> > > > > con2fb_release_oldinfo(), this free operation is protected by > > > > console_lock() in fbcon_set_con2fb_map_ioctl(), it also results in > > > > the change of certain states such as "minfo->dead" in matroxfb_remove(), > > > > so that it can be checked to avoid use-after-free before the use sites > > > > (e.g., the check at the beginning of matroxfb_ioctl()). However, > > > > the problem is that the use site is not protected by the same locks > > > > as for the free operation, e.g., "default" case in do_fb_ioctl() > > > > can lead to "matroxfb_ioctl()" but it's not protected by console_lock(), > > > > which can invalidate the aforementioned state set and check in a > > > > concurrent setting. > > > > > > > > Prevent the potential use-after-free issues by protecting the "default" > > > > case in do_fb_ioctl() with console_lock(), similarly as for many other > > > > cases like "case FBIOBLANK" and "case FBIOPAN_DISPLAY". > > > > > > > > Signed-off-by: Hang Zhang <zh.nvgt@gmail.com> > > > > > > applied to fbdev git tree. > > > > The patch above makes no sense at all to me: > > > > - fb_info is protected by lock_fb_info and > > - the lifetime of fb_info is protected by the get/put functions > > - yes there's the interaction with con2fb, which is protected by > > console_lock, but the lifetime guarantees are ensured by the device > > removal > > - which means any stuff happening in matroxfb_remove is also not a > > concern here (unless matroxfb completely gets all the device lifetime > > stuff wrong, but it doesn't look like it's any worse than any of the > > other fbdev drivers that we haven't recently fixed up due to the > > takeover issues with firmware drivers > > I have also a really hard timing finding the con2fb map use in the > matroxfb ioctl code, but that just might be that I didn't look > carefully enough. Maybe that would shed some light on this. > -Daniel > > > > > > On the very clear downside this now means we take console_lock for the > > vblank ioctl (which is a device driver extension for reasons, despite > > that it's a standard fbdev ioctl), which is no good at all given how > > console_lock() is a really expensive lock. > > > > Unless I'm massively missing something, can you pls push the revert > > before this lands in Linus' tree? > > > > Thanks, Daniel Hi, Daniel. Thank you for your feedback. We're not developers of the video subsystem and thus may be short in domain knowledge (e.g., the performance of console_lock() and the complex lifetime management). This patch initially intended to bring up the potential use-after-free issues here to the community - we have performed a best-effort code review and cannot exclude the possibility based on our understanding. What we have observed is that the call chain leading to the free site (do_fb_ioctl()->fbcon_set_con2fb_map_ioctl()->set_con2fb_map()-> con2fb_release_oldinfo()-> ... ->matroxfb_remove()) is only protected by console_lock() but not lock_fb_info(), while the potential use site (call chain starts from the default case in do_fb_ioctl()) is only protected by lock_fb_info() but not console_lock(). We thus propose to add this extra console_lock() to the default case, which is inspired by the lock protection of many other existing switch-case terms in the same function. Since we do not have deep domain knowledge of this subsystem, we will rely on the developers to make a decision regarding the patch. Thank you again for your review and help! Best, Hang > > > > > Thanks, > > > Helge > > > > > > > --- > > > > drivers/video/fbdev/core/fbmem.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > > > > index 1e70d8c67653..8b1a1527d18a 100644 > > > > --- a/drivers/video/fbdev/core/fbmem.c > > > > +++ b/drivers/video/fbdev/core/fbmem.c > > > > @@ -1182,6 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > > > > console_unlock(); > > > > break; > > > > default: > > > > + console_lock(); > > > > lock_fb_info(info); > > > > fb = info->fbops; > > > > if (fb->fb_ioctl) > > > > @@ -1189,6 +1190,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > > > > else > > > > ret = -ENOTTY; > > > > unlock_fb_info(info); > > > > + console_unlock(); > > > > } > > > > return ret; > > > > } > > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, Jan 05, 2023 at 01:38:54PM -0500, Hang Zhang wrote: > On Thu, Jan 5, 2023 at 5:25 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Thu, 5 Jan 2023 at 11:21, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > Hi Helge > > > > > > On Mon, 2 Jan 2023 at 16:28, Helge Deller <deller@gmx.de> wrote: > > > > > > > > On 12/30/22 07:35, Hang Zhang wrote: > > > > > In do_fb_ioctl(), user specified "fb_info" can be freed in the callee > > > > > fbcon_get_con2fb_map_ioctl() -> set_con2fb_map() -> > > > > > con2fb_release_oldinfo(), this free operation is protected by > > > > > console_lock() in fbcon_set_con2fb_map_ioctl(), it also results in > > > > > the change of certain states such as "minfo->dead" in matroxfb_remove(), > > > > > so that it can be checked to avoid use-after-free before the use sites > > > > > (e.g., the check at the beginning of matroxfb_ioctl()). However, > > > > > the problem is that the use site is not protected by the same locks > > > > > as for the free operation, e.g., "default" case in do_fb_ioctl() > > > > > can lead to "matroxfb_ioctl()" but it's not protected by console_lock(), > > > > > which can invalidate the aforementioned state set and check in a > > > > > concurrent setting. > > > > > > > > > > Prevent the potential use-after-free issues by protecting the "default" > > > > > case in do_fb_ioctl() with console_lock(), similarly as for many other > > > > > cases like "case FBIOBLANK" and "case FBIOPAN_DISPLAY". > > > > > > > > > > Signed-off-by: Hang Zhang <zh.nvgt@gmail.com> > > > > > > > > applied to fbdev git tree. > > > > > > The patch above makes no sense at all to me: > > > > > > - fb_info is protected by lock_fb_info and > > > - the lifetime of fb_info is protected by the get/put functions > > > - yes there's the interaction with con2fb, which is protected by > > > console_lock, but the lifetime guarantees are ensured by the device > > > removal > > > - which means any stuff happening in matroxfb_remove is also not a > > > concern here (unless matroxfb completely gets all the device lifetime > > > stuff wrong, but it doesn't look like it's any worse than any of the > > > other fbdev drivers that we haven't recently fixed up due to the > > > takeover issues with firmware drivers > > > > I have also a really hard timing finding the con2fb map use in the > > matroxfb ioctl code, but that just might be that I didn't look > > carefully enough. Maybe that would shed some light on this. > > -Daniel > > > > > > > > > > On the very clear downside this now means we take console_lock for the > > > vblank ioctl (which is a device driver extension for reasons, despite > > > that it's a standard fbdev ioctl), which is no good at all given how > > > console_lock() is a really expensive lock. > > > > > > Unless I'm massively missing something, can you pls push the revert > > > before this lands in Linus' tree? > > > > > > Thanks, Daniel > > Hi, Daniel. Thank you for your feedback. We're not developers of the > video subsystem and thus may be short in domain knowledge (e.g., the > performance of console_lock() and the complex lifetime management). > This patch initially intended to bring up the potential use-after-free > issues here to the community - we have performed a best-effort code > review and cannot exclude the possibility based on our understanding. > > What we have observed is that the call chain leading to the free site > (do_fb_ioctl()->fbcon_set_con2fb_map_ioctl()->set_con2fb_map()-> > con2fb_release_oldinfo()-> ... ->matroxfb_remove()) is only protected > by console_lock() but not lock_fb_info(), while the potential use > site (call chain starts from the default case in do_fb_ioctl()) is > only protected by lock_fb_info() but not console_lock(). > We thus propose to add this extra console_lock() to the default case, > which is inspired by the lock protection of many other existing > switch-case terms in the same function. > > Since we do not have deep domain knowledge of this subsystem, we will > rely on the developers to make a decision regarding the patch. Thank > you again for your review and help! Can you please elaborate where you've found this use-after-free and how? I'm still not understanding how you even got here - this is orthogonal to whether the patch is the right fix or not. -Daniel > > Best, > Hang > > > > > > > > Thanks, > > > > Helge > > > > > > > > > --- > > > > > drivers/video/fbdev/core/fbmem.c | 2 ++ > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > > > > > index 1e70d8c67653..8b1a1527d18a 100644 > > > > > --- a/drivers/video/fbdev/core/fbmem.c > > > > > +++ b/drivers/video/fbdev/core/fbmem.c > > > > > @@ -1182,6 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > > > > > console_unlock(); > > > > > break; > > > > > default: > > > > > + console_lock(); > > > > > lock_fb_info(info); > > > > > fb = info->fbops; > > > > > if (fb->fb_ioctl) > > > > > @@ -1189,6 +1190,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > > > > > else > > > > > ret = -ENOTTY; > > > > > unlock_fb_info(info); > > > > > + console_unlock(); > > > > > } > > > > > return ret; > > > > > } > > > > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
On Fri, Jan 6, 2023 at 1:59 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Jan 05, 2023 at 01:38:54PM -0500, Hang Zhang wrote: > > On Thu, Jan 5, 2023 at 5:25 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > On Thu, 5 Jan 2023 at 11:21, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > > > Hi Helge > > > > > > > > On Mon, 2 Jan 2023 at 16:28, Helge Deller <deller@gmx.de> wrote: > > > > > > > > > > On 12/30/22 07:35, Hang Zhang wrote: > > > > > > In do_fb_ioctl(), user specified "fb_info" can be freed in the callee > > > > > > fbcon_get_con2fb_map_ioctl() -> set_con2fb_map() -> > > > > > > con2fb_release_oldinfo(), this free operation is protected by > > > > > > console_lock() in fbcon_set_con2fb_map_ioctl(), it also results in > > > > > > the change of certain states such as "minfo->dead" in matroxfb_remove(), > > > > > > so that it can be checked to avoid use-after-free before the use sites > > > > > > (e.g., the check at the beginning of matroxfb_ioctl()). However, > > > > > > the problem is that the use site is not protected by the same locks > > > > > > as for the free operation, e.g., "default" case in do_fb_ioctl() > > > > > > can lead to "matroxfb_ioctl()" but it's not protected by console_lock(), > > > > > > which can invalidate the aforementioned state set and check in a > > > > > > concurrent setting. > > > > > > > > > > > > Prevent the potential use-after-free issues by protecting the "default" > > > > > > case in do_fb_ioctl() with console_lock(), similarly as for many other > > > > > > cases like "case FBIOBLANK" and "case FBIOPAN_DISPLAY". > > > > > > > > > > > > Signed-off-by: Hang Zhang <zh.nvgt@gmail.com> > > > > > > > > > > applied to fbdev git tree. > > > > > > > > The patch above makes no sense at all to me: > > > > > > > > - fb_info is protected by lock_fb_info and > > > > - the lifetime of fb_info is protected by the get/put functions > > > > - yes there's the interaction with con2fb, which is protected by > > > > console_lock, but the lifetime guarantees are ensured by the device > > > > removal > > > > - which means any stuff happening in matroxfb_remove is also not a > > > > concern here (unless matroxfb completely gets all the device lifetime > > > > stuff wrong, but it doesn't look like it's any worse than any of the > > > > other fbdev drivers that we haven't recently fixed up due to the > > > > takeover issues with firmware drivers > > > > > > I have also a really hard timing finding the con2fb map use in the > > > matroxfb ioctl code, but that just might be that I didn't look > > > carefully enough. Maybe that would shed some light on this. > > > -Daniel > > > > > > > > > > > > > > On the very clear downside this now means we take console_lock for the > > > > vblank ioctl (which is a device driver extension for reasons, despite > > > > that it's a standard fbdev ioctl), which is no good at all given how > > > > console_lock() is a really expensive lock. > > > > > > > > Unless I'm massively missing something, can you pls push the revert > > > > before this lands in Linus' tree? > > > > > > > > Thanks, Daniel > > > > Hi, Daniel. Thank you for your feedback. We're not developers of the > > video subsystem and thus may be short in domain knowledge (e.g., the > > performance of console_lock() and the complex lifetime management). > > This patch initially intended to bring up the potential use-after-free > > issues here to the community - we have performed a best-effort code > > review and cannot exclude the possibility based on our understanding. > > > > What we have observed is that the call chain leading to the free site > > (do_fb_ioctl()->fbcon_set_con2fb_map_ioctl()->set_con2fb_map()-> > > con2fb_release_oldinfo()-> ... ->matroxfb_remove()) is only protected > > by console_lock() but not lock_fb_info(), while the potential use > > site (call chain starts from the default case in do_fb_ioctl()) is > > only protected by lock_fb_info() but not console_lock(). > > We thus propose to add this extra console_lock() to the default case, > > which is inspired by the lock protection of many other existing > > switch-case terms in the same function. > > > > Since we do not have deep domain knowledge of this subsystem, we will > > rely on the developers to make a decision regarding the patch. Thank > > you again for your review and help! > > Can you please elaborate where you've found this use-after-free and how? > I'm still not understanding how you even got here - this is orthogonal to > whether the patch is the right fix or not. > -Daniel Hi, Daniel. Sure. This issue was initially flagged by our experimental static code analyzer aiming to find use-after-free issues in the kernel - that's why we don't have PoC or execution traces here. We deeply understand that static analyzer can generate false alarms, so we have tried our best and spent a good amount of time carefully reviewing the related code. We eventually found that we could not exclude this potential issue based on our study, so we decided to report this to the community with this tentative fix. As mentioned, we may be short in domain knowledge, so your input is highly appreciated. We respect the developer's decision about whether this is really a problem and whether/how to fix it. However, if you think the use-after-free is actually not possible, it will be very helpful if you can elaborate on the reasoning since it will greatly help us improve our analyzer. Thank you very much for your help! BTW, if this is worthed a fix and the performance of console_lock() is a major concern, then I think there may be alternative solutions like adding a lock_fb_info() to the free call chain - if that's better in performance, or maybe selectively protect the matroxfb ioctl but not vblank ioctl as you mentioned. Thanks, Hang > > > > > Best, > > Hang > > > > > > > > > > > Thanks, > > > > > Helge > > > > > > > > > > > --- > > > > > > drivers/video/fbdev/core/fbmem.c | 2 ++ > > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > > > > > > index 1e70d8c67653..8b1a1527d18a 100644 > > > > > > --- a/drivers/video/fbdev/core/fbmem.c > > > > > > +++ b/drivers/video/fbdev/core/fbmem.c > > > > > > @@ -1182,6 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > > > > > > console_unlock(); > > > > > > break; > > > > > > default: > > > > > > + console_lock(); > > > > > > lock_fb_info(info); > > > > > > fb = info->fbops; > > > > > > if (fb->fb_ioctl) > > > > > > @@ -1189,6 +1190,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > > > > > > else > > > > > > ret = -ENOTTY; > > > > > > unlock_fb_info(info); > > > > > > + console_unlock(); > > > > > > } > > > > > > return ret; > > > > > > } > > > > > > > > > > > > > > > > > -- > > > > Daniel Vetter > > > > Software Engineer, Intel Corporation > > > > http://blog.ffwll.ch > > > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Fri, Jan 06, 2023 at 02:58:27PM -0500, Hang Zhang wrote: > On Fri, Jan 6, 2023 at 1:59 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Thu, Jan 05, 2023 at 01:38:54PM -0500, Hang Zhang wrote: > > > On Thu, Jan 5, 2023 at 5:25 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > > > On Thu, 5 Jan 2023 at 11:21, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > > > > > Hi Helge > > > > > > > > > > On Mon, 2 Jan 2023 at 16:28, Helge Deller <deller@gmx.de> wrote: > > > > > > > > > > > > On 12/30/22 07:35, Hang Zhang wrote: > > > > > > > In do_fb_ioctl(), user specified "fb_info" can be freed in the callee > > > > > > > fbcon_get_con2fb_map_ioctl() -> set_con2fb_map() -> > > > > > > > con2fb_release_oldinfo(), this free operation is protected by > > > > > > > console_lock() in fbcon_set_con2fb_map_ioctl(), it also results in > > > > > > > the change of certain states such as "minfo->dead" in matroxfb_remove(), > > > > > > > so that it can be checked to avoid use-after-free before the use sites > > > > > > > (e.g., the check at the beginning of matroxfb_ioctl()). However, > > > > > > > the problem is that the use site is not protected by the same locks > > > > > > > as for the free operation, e.g., "default" case in do_fb_ioctl() > > > > > > > can lead to "matroxfb_ioctl()" but it's not protected by console_lock(), > > > > > > > which can invalidate the aforementioned state set and check in a > > > > > > > concurrent setting. > > > > > > > > > > > > > > Prevent the potential use-after-free issues by protecting the "default" > > > > > > > case in do_fb_ioctl() with console_lock(), similarly as for many other > > > > > > > cases like "case FBIOBLANK" and "case FBIOPAN_DISPLAY". > > > > > > > > > > > > > > Signed-off-by: Hang Zhang <zh.nvgt@gmail.com> > > > > > > > > > > > > applied to fbdev git tree. > > > > > > > > > > The patch above makes no sense at all to me: > > > > > > > > > > - fb_info is protected by lock_fb_info and > > > > > - the lifetime of fb_info is protected by the get/put functions > > > > > - yes there's the interaction with con2fb, which is protected by > > > > > console_lock, but the lifetime guarantees are ensured by the device > > > > > removal > > > > > - which means any stuff happening in matroxfb_remove is also not a > > > > > concern here (unless matroxfb completely gets all the device lifetime > > > > > stuff wrong, but it doesn't look like it's any worse than any of the > > > > > other fbdev drivers that we haven't recently fixed up due to the > > > > > takeover issues with firmware drivers > > > > > > > > I have also a really hard timing finding the con2fb map use in the > > > > matroxfb ioctl code, but that just might be that I didn't look > > > > carefully enough. Maybe that would shed some light on this. > > > > -Daniel > > > > > > > > > > > > > > > > > > On the very clear downside this now means we take console_lock for the > > > > > vblank ioctl (which is a device driver extension for reasons, despite > > > > > that it's a standard fbdev ioctl), which is no good at all given how > > > > > console_lock() is a really expensive lock. > > > > > > > > > > Unless I'm massively missing something, can you pls push the revert > > > > > before this lands in Linus' tree? > > > > > > > > > > Thanks, Daniel > > > > > > Hi, Daniel. Thank you for your feedback. We're not developers of the > > > video subsystem and thus may be short in domain knowledge (e.g., the > > > performance of console_lock() and the complex lifetime management). > > > This patch initially intended to bring up the potential use-after-free > > > issues here to the community - we have performed a best-effort code > > > review and cannot exclude the possibility based on our understanding. > > > > > > What we have observed is that the call chain leading to the free site > > > (do_fb_ioctl()->fbcon_set_con2fb_map_ioctl()->set_con2fb_map()-> > > > con2fb_release_oldinfo()-> ... ->matroxfb_remove()) is only protected > > > by console_lock() but not lock_fb_info(), while the potential use > > > site (call chain starts from the default case in do_fb_ioctl()) is > > > only protected by lock_fb_info() but not console_lock(). > > > We thus propose to add this extra console_lock() to the default case, > > > which is inspired by the lock protection of many other existing > > > switch-case terms in the same function. > > > > > > Since we do not have deep domain knowledge of this subsystem, we will > > > rely on the developers to make a decision regarding the patch. Thank > > > you again for your review and help! > > > > Can you please elaborate where you've found this use-after-free and how? > > I'm still not understanding how you even got here - this is orthogonal to > > whether the patch is the right fix or not. > > -Daniel > > Hi, Daniel. Sure. This issue was initially flagged by our experimental static > code analyzer aiming to find use-after-free issues in the kernel - that's why > we don't have PoC or execution traces here. We deeply understand that > static analyzer can generate false alarms, so we have tried our best and > spent a good amount of time carefully reviewing the related code. We > eventually found that we could not exclude this potential issue based on our > study, so we decided to report this to the community with this tentative fix. As > mentioned, we may be short in domain knowledge, so your input is > highly appreciated. We respect the developer's decision about whether > this is really a problem and whether/how to fix it. However, if you think the > use-after-free is actually not possible, it will be very helpful if you can > elaborate on the reasoning since it will greatly help us improve our > analyzer. Thank you very much for your help! Please start out these patches with the fact that this is from an experimental checker. Also do include _why_ your checker things something is going wrong. If you cannot follow why the checker complains about something, then don't report it as an issue until you do. Also, if you do not understand the code, please make it absolutely clear that you just guessed a possible fix, but not that it's been tested in any way or form. If you don't do this, then we end up wasting a ton of time of people who don't have surplus time, because in this case the patch got review, applied, pull request made, I realized it looks funny, patch dropped, pull request remade, and then a fairly big thread here. All for a bug that's likely in your checker and not in the kernel. This is not great. > BTW, if this is worthed a fix and the performance of console_lock() is a > major concern, then I think there may be alternative solutions like adding > a lock_fb_info() to the free call chain - if that's better in performance, > or maybe selectively protect the matroxfb ioctl but not vblank ioctl as you > mentioned. Please start out with explaining what kind of bug your checker is seeing, and why. Not how you're trying to fix it. Because I'm pretty sure there isn't a bug, but since I've already spent a pile of time looking at this, I want to make sure. Cheers, Daniel
On Fri, Jan 6, 2023 at 3:05 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Jan 06, 2023 at 02:58:27PM -0500, Hang Zhang wrote: > > On Fri, Jan 6, 2023 at 1:59 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > On Thu, Jan 05, 2023 at 01:38:54PM -0500, Hang Zhang wrote: > > > > On Thu, Jan 5, 2023 at 5:25 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > > > > > On Thu, 5 Jan 2023 at 11:21, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > > > > > > > Hi Helge > > > > > > > > > > > > On Mon, 2 Jan 2023 at 16:28, Helge Deller <deller@gmx.de> wrote: > > > > > > > > > > > > > > On 12/30/22 07:35, Hang Zhang wrote: > > > > > > > > In do_fb_ioctl(), user specified "fb_info" can be freed in the callee > > > > > > > > fbcon_get_con2fb_map_ioctl() -> set_con2fb_map() -> > > > > > > > > con2fb_release_oldinfo(), this free operation is protected by > > > > > > > > console_lock() in fbcon_set_con2fb_map_ioctl(), it also results in > > > > > > > > the change of certain states such as "minfo->dead" in matroxfb_remove(), > > > > > > > > so that it can be checked to avoid use-after-free before the use sites > > > > > > > > (e.g., the check at the beginning of matroxfb_ioctl()). However, > > > > > > > > the problem is that the use site is not protected by the same locks > > > > > > > > as for the free operation, e.g., "default" case in do_fb_ioctl() > > > > > > > > can lead to "matroxfb_ioctl()" but it's not protected by console_lock(), > > > > > > > > which can invalidate the aforementioned state set and check in a > > > > > > > > concurrent setting. > > > > > > > > > > > > > > > > Prevent the potential use-after-free issues by protecting the "default" > > > > > > > > case in do_fb_ioctl() with console_lock(), similarly as for many other > > > > > > > > cases like "case FBIOBLANK" and "case FBIOPAN_DISPLAY". > > > > > > > > > > > > > > > > Signed-off-by: Hang Zhang <zh.nvgt@gmail.com> > > > > > > > > > > > > > > applied to fbdev git tree. > > > > > > > > > > > > The patch above makes no sense at all to me: > > > > > > > > > > > > - fb_info is protected by lock_fb_info and > > > > > > - the lifetime of fb_info is protected by the get/put functions > > > > > > - yes there's the interaction with con2fb, which is protected by > > > > > > console_lock, but the lifetime guarantees are ensured by the device > > > > > > removal > > > > > > - which means any stuff happening in matroxfb_remove is also not a > > > > > > concern here (unless matroxfb completely gets all the device lifetime > > > > > > stuff wrong, but it doesn't look like it's any worse than any of the > > > > > > other fbdev drivers that we haven't recently fixed up due to the > > > > > > takeover issues with firmware drivers > > > > > > > > > > I have also a really hard timing finding the con2fb map use in the > > > > > matroxfb ioctl code, but that just might be that I didn't look > > > > > carefully enough. Maybe that would shed some light on this. > > > > > -Daniel > > > > > > > > > > > > > > > > > > > > > > On the very clear downside this now means we take console_lock for the > > > > > > vblank ioctl (which is a device driver extension for reasons, despite > > > > > > that it's a standard fbdev ioctl), which is no good at all given how > > > > > > console_lock() is a really expensive lock. > > > > > > > > > > > > Unless I'm massively missing something, can you pls push the revert > > > > > > before this lands in Linus' tree? > > > > > > > > > > > > Thanks, Daniel > > > > > > > > Hi, Daniel. Thank you for your feedback. We're not developers of the > > > > video subsystem and thus may be short in domain knowledge (e.g., the > > > > performance of console_lock() and the complex lifetime management). > > > > This patch initially intended to bring up the potential use-after-free > > > > issues here to the community - we have performed a best-effort code > > > > review and cannot exclude the possibility based on our understanding. > > > > > > > > What we have observed is that the call chain leading to the free site > > > > (do_fb_ioctl()->fbcon_set_con2fb_map_ioctl()->set_con2fb_map()-> > > > > con2fb_release_oldinfo()-> ... ->matroxfb_remove()) is only protected > > > > by console_lock() but not lock_fb_info(), while the potential use > > > > site (call chain starts from the default case in do_fb_ioctl()) is > > > > only protected by lock_fb_info() but not console_lock(). > > > > We thus propose to add this extra console_lock() to the default case, > > > > which is inspired by the lock protection of many other existing > > > > switch-case terms in the same function. > > > > > > > > Since we do not have deep domain knowledge of this subsystem, we will > > > > rely on the developers to make a decision regarding the patch. Thank > > > > you again for your review and help! > > > > > > Can you please elaborate where you've found this use-after-free and how? > > > I'm still not understanding how you even got here - this is orthogonal to > > > whether the patch is the right fix or not. > > > -Daniel > > > > Hi, Daniel. Sure. This issue was initially flagged by our experimental static > > code analyzer aiming to find use-after-free issues in the kernel - that's why > > we don't have PoC or execution traces here. We deeply understand that > > static analyzer can generate false alarms, so we have tried our best and > > spent a good amount of time carefully reviewing the related code. We > > eventually found that we could not exclude this potential issue based on our > > study, so we decided to report this to the community with this tentative fix. As > > mentioned, we may be short in domain knowledge, so your input is > > highly appreciated. We respect the developer's decision about whether > > this is really a problem and whether/how to fix it. However, if you think the > > use-after-free is actually not possible, it will be very helpful if you can > > elaborate on the reasoning since it will greatly help us improve our > > analyzer. Thank you very much for your help! > > Please start out these patches with the fact that this is from an > experimental checker. > > Also do include _why_ your checker things something is going wrong. If you > cannot follow why the checker complains about something, then don't report > it as an issue until you do. Also, if you do not understand the code, > please make it absolutely clear that you just guessed a possible fix, but > not that it's been tested in any way or form. > > If you don't do this, then we end up wasting a ton of time of people who > don't have surplus time, because in this case the patch got review, > applied, pull request made, I realized it looks funny, patch dropped, pull > request remade, and then a fairly big thread here. > > All for a bug that's likely in your checker and not in the kernel. This is > not great. > > > BTW, if this is worthed a fix and the performance of console_lock() is a > > major concern, then I think there may be alternative solutions like adding > > a lock_fb_info() to the free call chain - if that's better in performance, > > or maybe selectively protect the matroxfb ioctl but not vblank ioctl as you > > mentioned. > > Please start out with explaining what kind of bug your checker is seeing, > and why. Not how you're trying to fix it. Because I'm pretty sure there > isn't a bug, but since I've already spent a pile of time looking at this, > I want to make sure. > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch We are sorry for the inconvenience caused, we'll follow these practices and guidelines in the future. Thank you!
On Fri, Jan 06, 2023 at 03:25:14PM -0500, Hang Zhang wrote: > On Fri, Jan 6, 2023 at 3:05 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Jan 06, 2023 at 02:58:27PM -0500, Hang Zhang wrote: > > > On Fri, Jan 6, 2023 at 1:59 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > BTW, if this is worthed a fix and the performance of console_lock() is a > > > major concern, then I think there may be alternative solutions like adding > > > a lock_fb_info() to the free call chain - if that's better in performance, > > > or maybe selectively protect the matroxfb ioctl but not vblank ioctl as you > > > mentioned. > > > > Please start out with explaining what kind of bug your checker is seeing, > > and why. Not how you're trying to fix it. Because I'm pretty sure there > > isn't a bug, but since I've already spent a pile of time looking at this, > > I want to make sure. > > We are sorry for the inconvenience caused, we'll follow these practices and > guidelines in the future. Thank you! Once more: Please explain what you're static checker is seeing. I want to understanding this, and I'm hoping at least someone involved in this static checker can explain what it thinks is going on. Thanks, Daniel
On Fri, Jan 6, 2023 at 4:19 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Jan 06, 2023 at 03:25:14PM -0500, Hang Zhang wrote: > > On Fri, Jan 6, 2023 at 3:05 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Fri, Jan 06, 2023 at 02:58:27PM -0500, Hang Zhang wrote: > > > > On Fri, Jan 6, 2023 at 1:59 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > BTW, if this is worthed a fix and the performance of console_lock() is a > > > > major concern, then I think there may be alternative solutions like adding > > > > a lock_fb_info() to the free call chain - if that's better in performance, > > > > or maybe selectively protect the matroxfb ioctl but not vblank ioctl as you > > > > mentioned. > > > > > > Please start out with explaining what kind of bug your checker is seeing, > > > and why. Not how you're trying to fix it. Because I'm pretty sure there > > > isn't a bug, but since I've already spent a pile of time looking at this, > > > I want to make sure. > > > > We are sorry for the inconvenience caused, we'll follow these practices and > > guidelines in the future. Thank you! > > Once more: Please explain what you're static checker is seeing. I want to > understanding this, and I'm hoping at least someone involved in this > static checker can explain what it thinks is going on. > > Thanks, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch Thank you for your interest, Daniel. The checker tries first to find the free and use sites of a certain object (in this case "fb_info"), then reason about whether the use can actually happen after the free (e.g., taking into account factors like state set/check, locks, etc.), if so, it will flag a potential use-after-free. As a static checker, is doesn't execute a program or generate a PoC. We then manually review each flagged issue by inspecting all related code. In this case, the checker (and us) are unaware of the lifetime management logic, which may cause problems. Best, Hang
On Fri, Jan 06, 2023 at 05:12:57PM -0500, Hang Zhang wrote: > On Fri, Jan 6, 2023 at 4:19 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Jan 06, 2023 at 03:25:14PM -0500, Hang Zhang wrote: > > > On Fri, Jan 6, 2023 at 3:05 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Fri, Jan 06, 2023 at 02:58:27PM -0500, Hang Zhang wrote: > > > > > On Fri, Jan 6, 2023 at 1:59 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > BTW, if this is worthed a fix and the performance of console_lock() is a > > > > > major concern, then I think there may be alternative solutions like adding > > > > > a lock_fb_info() to the free call chain - if that's better in performance, > > > > > or maybe selectively protect the matroxfb ioctl but not vblank ioctl as you > > > > > mentioned. > > > > > > > > Please start out with explaining what kind of bug your checker is seeing, > > > > and why. Not how you're trying to fix it. Because I'm pretty sure there > > > > isn't a bug, but since I've already spent a pile of time looking at this, > > > > I want to make sure. > > > > > > We are sorry for the inconvenience caused, we'll follow these practices and > > > guidelines in the future. Thank you! > > > > Once more: Please explain what you're static checker is seeing. I want to > > understanding this, and I'm hoping at least someone involved in this > > static checker can explain what it thinks is going on. > > > > Thanks, Daniel > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > Thank you for your interest, Daniel. The checker tries first to find > the free and > use sites of a certain object (in this case "fb_info"), then reason > about whether > the use can actually happen after the free (e.g., taking into account > factors like > state set/check, locks, etc.), if so, it will flag a potential > use-after-free. As a static > checker, is doesn't execute a program or generate a PoC. We then manually > review each flagged issue by inspecting all related code. In this > case, the checker > (and us) are unaware of the lifetime management logic, which may cause > problems. Lifetime management is and absolute basic part in the linux kernel. So if your checker flags every free which isn't protected by a lock, then you'll creating endless amounts of false positives. Is this really what you're doing? I'm still very confused ... -Daniel
On Fri, Jan 6, 2023 at 5:46 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Jan 06, 2023 at 05:12:57PM -0500, Hang Zhang wrote: > > On Fri, Jan 6, 2023 at 4:19 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Fri, Jan 06, 2023 at 03:25:14PM -0500, Hang Zhang wrote: > > > > On Fri, Jan 6, 2023 at 3:05 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Fri, Jan 06, 2023 at 02:58:27PM -0500, Hang Zhang wrote: > > > > > > On Fri, Jan 6, 2023 at 1:59 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > BTW, if this is worthed a fix and the performance of console_lock() is a > > > > > > major concern, then I think there may be alternative solutions like adding > > > > > > a lock_fb_info() to the free call chain - if that's better in performance, > > > > > > or maybe selectively protect the matroxfb ioctl but not vblank ioctl as you > > > > > > mentioned. > > > > > > > > > > Please start out with explaining what kind of bug your checker is seeing, > > > > > and why. Not how you're trying to fix it. Because I'm pretty sure there > > > > > isn't a bug, but since I've already spent a pile of time looking at this, > > > > > I want to make sure. > > > > > > > > We are sorry for the inconvenience caused, we'll follow these practices and > > > > guidelines in the future. Thank you! > > > > > > Once more: Please explain what you're static checker is seeing. I want to > > > understanding this, and I'm hoping at least someone involved in this > > > static checker can explain what it thinks is going on. > > > > > > Thanks, Daniel > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > Thank you for your interest, Daniel. The checker tries first to find > > the free and > > use sites of a certain object (in this case "fb_info"), then reason > > about whether > > the use can actually happen after the free (e.g., taking into account > > factors like > > state set/check, locks, etc.), if so, it will flag a potential > > use-after-free. As a static > > checker, is doesn't execute a program or generate a PoC. We then manually > > review each flagged issue by inspecting all related code. In this > > case, the checker > > (and us) are unaware of the lifetime management logic, which may cause > > problems. > > Lifetime management is and absolute basic part in the linux kernel. So if > your checker flags every free which isn't protected by a lock, then you'll > creating endless amounts of false positives. Is this really what you're > doing? > > I'm still very confused ... > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch Hi, Daniel. Lock is only one of many factors we consider in the checker, so the actual workflow is certainly more complicated than "mark every free w/o lock". E.g., we also need to consider the data flow between use and free, the state check, etc. But as you know, it is very challenging for a static checker to automatically and accurately reason about everything in the code (automatic lifetime management analysis can also be tricky for a static analyzer), that's why we perform a manual review afterward. We will continue working on the checker to reduce its false alarms and submit higher-quality reports to the community following your guidelines. Thank you so much for your time! Best, Hang
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 1e70d8c67653..8b1a1527d18a 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1182,6 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, console_unlock(); break; default: + console_lock(); lock_fb_info(info); fb = info->fbops; if (fb->fb_ioctl) @@ -1189,6 +1190,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, else ret = -ENOTTY; unlock_fb_info(info); + console_unlock(); } return ret; }
In do_fb_ioctl(), user specified "fb_info" can be freed in the callee fbcon_get_con2fb_map_ioctl() -> set_con2fb_map() -> con2fb_release_oldinfo(), this free operation is protected by console_lock() in fbcon_set_con2fb_map_ioctl(), it also results in the change of certain states such as "minfo->dead" in matroxfb_remove(), so that it can be checked to avoid use-after-free before the use sites (e.g., the check at the beginning of matroxfb_ioctl()). However, the problem is that the use site is not protected by the same locks as for the free operation, e.g., "default" case in do_fb_ioctl() can lead to "matroxfb_ioctl()" but it's not protected by console_lock(), which can invalidate the aforementioned state set and check in a concurrent setting. Prevent the potential use-after-free issues by protecting the "default" case in do_fb_ioctl() with console_lock(), similarly as for many other cases like "case FBIOBLANK" and "case FBIOPAN_DISPLAY". Signed-off-by: Hang Zhang <zh.nvgt@gmail.com> --- drivers/video/fbdev/core/fbmem.c | 2 ++ 1 file changed, 2 insertions(+)