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 <[email protected]>
---
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;
}
--
2.39.0
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 <[email protected]>
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;
> }
Hi Helge
On Mon, 2 Jan 2023 at 16:28, Helge Deller <[email protected]> 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 <[email protected]>
>
> 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
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
> 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
On Thu, 5 Jan 2023 at 11:21, Daniel Vetter <[email protected]> wrote:
>
> Hi Helge
>
> On Mon, 2 Jan 2023 at 16:28, Helge Deller <[email protected]> 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 <[email protected]>
> >
> > 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
>
> > 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 5, 2023 at 5:25 AM Daniel Vetter <[email protected]> wrote:
>
> On Thu, 5 Jan 2023 at 11:21, Daniel Vetter <[email protected]> wrote:
> >
> > Hi Helge
> >
> > On Mon, 2 Jan 2023 at 16:28, Helge Deller <[email protected]> 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 <[email protected]>
> > >
> > > 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 <[email protected]> wrote:
> >
> > On Thu, 5 Jan 2023 at 11:21, Daniel Vetter <[email protected]> wrote:
> > >
> > > Hi Helge
> > >
> > > On Mon, 2 Jan 2023 at 16:28, Helge Deller <[email protected]> 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 <[email protected]>
> > > >
> > > > 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
--
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 <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > On Thu, 5 Jan 2023 at 11:21, Daniel Vetter <[email protected]> wrote:
> > > > >
> > > > > Hi Helge
> > > > >
> > > > > On Mon, 2 Jan 2023 at 16:28, Helge Deller <[email protected]> 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 <[email protected]>
> > > > > >
> > > > > > 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
On Fri, Jan 6, 2023 at 1:59 PM Daniel Vetter <[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Thu, 5 Jan 2023 at 11:21, Daniel Vetter <[email protected]> wrote:
> > > >
> > > > Hi Helge
> > > >
> > > > On Mon, 2 Jan 2023 at 16:28, Helge Deller <[email protected]> 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 <[email protected]>
> > > > >
> > > > > 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 6, 2023 at 3:05 PM Daniel Vetter <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> > > > >
> > > > > On Thu, 5 Jan 2023 at 11:21, Daniel Vetter <[email protected]> wrote:
> > > > > >
> > > > > > Hi Helge
> > > > > >
> > > > > > On Mon, 2 Jan 2023 at 16:28, Helge Deller <[email protected]> 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 <[email protected]>
> > > > > > >
> > > > > > > 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 <[email protected]> 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 <[email protected]> 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
On Fri, Jan 6, 2023 at 4:19 PM Daniel Vetter <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
On Fri, Jan 6, 2023 at 5:46 PM Daniel Vetter <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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