On Fri, Jan 6, 2023 at 1:59 PM Daniel Vetter [off-list ref] wrote:

On Thu, Jan 05, 2023 at 01:38:54PM -0500, Hang Zhang wrote:

quoted

On Thu, Jan 5, 2023 at 5:25 AM Daniel Vetter [off-list ref] wrote:

quoted

On Thu, 5 Jan 2023 at 11:21, Daniel Vetter [off-list ref] wrote:

quoted

Hi Helge

On Mon, 2 Jan 2023 at 16:28, Helge Deller [off-list ref] wrote:

quoted

On 12/30/22 07:35, Hang Zhang wrote:

quoted

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 <redacted>

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

quoted

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

quoted

Best,
Hang

quoted

quoted

quoted

Thanks,
Helge

quoted

---
  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