[PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
Peilin Ye
yepeilin.cs at gmail.com
Wed Sep 30 07:11:51 UTC 2020
On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote:
> On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye <yepeilin.cs at gmail.com> wrote:
> > It seems that users don't use `console_font` directly, they use
> > `console_font_op`. Then, in TTY:
>
> Wow, this is a maze :-/
>
> > (drivers/tty/vt/vt.c)
> > int con_font_op(struct vc_data *vc, struct console_font_op *op)
> > {
> > switch (op->op) {
> > case KD_FONT_OP_SET:
> > return con_font_set(vc, op);
> > case KD_FONT_OP_GET:
> > return con_font_get(vc, op);
> > case KD_FONT_OP_SET_DEFAULT:
> > return con_font_default(vc, op);
> > case KD_FONT_OP_COPY:
> > return con_font_copy(vc, op);
> > }
> > return -ENOSYS;
> > }
>
> So my gut feeling is that this is just a bit of overenthusiastic
> common code sharing, and all it results is confuse everyone. I think
> if we change the conf_font_get/set/default/copy functions to not take
> the *op struct (which is take pretty arbitrarily from one of the
> ioctl), but the parameters each needs directly, that would clean up
> the code a _lot_. Since most callers would then directly call the
> right operation, instead of this detour through console_font_op.
> struct console_font_op is an uapi struct, so really shouldn't be used
> for internal abstractions - we can't change uapi, hence this makes it
> impossible to refactor anything from the get-go.
>
> I also think that trying to get rid of con_font_op callers as much as
> possible (everywhere where the op struct is constructed in the kernel
> and doesn't come from userspace essentially) should be doable as a
> stand-alone patch series.
I see, I'll do some code searching and try to clean them up.
> > These 4 functions allocate `console_font`. We can replace them with our
> > `kernel_console_font`. So, ...
> >
> > $ vgrep "\.con_font_set"
>
> An aside: git grep is awesome, and really fast.
Ah, yes, by default vgrep uses git-grep. I use vgrep when I need to see
something colorful :)
> > $ vgrep "\.con_font_get"
> > Index File Line Content
> > 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1295 .con_font_get = sisusbcon_font_get,
> > 1 drivers/video/console/vgacon.c 1227 .con_font_get = vgacon_font_get,
> > 2 drivers/video/fbdev/core/fbcon.c 3121 .con_font_get = fbcon_get_font,
> > $
> > $ vgrep "\.con_font_default"
> > Index File Line Content
> > 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1379 .con_font_default = sisusbdummycon_font_default,
> > 1 drivers/video/console/dummycon.c 163 .con_font_default = dummycon_font_default,
>
> The above two return 0 but do nothing, which means width/height are
> now bogus (or well the same as what userspace set). I don't think that
> works correctly ...
>
> > 2 drivers/video/console/newport_con.c 694 .con_font_default = newport_font_default,
>
> This just seems to release the userspace font. This is already done in
> other places where it makes a lot more sense to clean up.
>
> > 3 drivers/video/fbdev/core/fbcon.c 3122 .con_font_default = fbcon_set_def_font,
>
> This actually does something. tbh I would not be surprises if the
> fb_set utility is the only thing that uses this - with a bit of code
> search we could perhaps confirm this, and delete all the other
> implementations.
>
> > $ vgrep "\.con_font_copy"
> > Index File Line Content
> > 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1380 .con_font_copy = sisusbdummycon_font_copy,
> > 1 drivers/video/console/dummycon.c 164 .con_font_copy = dummycon_font_copy,
>
> Above two do nothing, but return 0. Again this wont work I think.
>
> > 2 drivers/video/fbdev/core/fbcon.c 3123 .con_font_copy = fbcon_copy_font,
>
> Smells again like something that's only used by fb_set, and we could
> probably delete the other dummy implementations. Also I'm not even
> really clear on what this does ...
>
> Removing these dummy functions means that for a dummy console these
> ioctls would start failing, but then I don't think anyone boots up
> into a dummy console and expects font changes to work. So again I
> think we could split this cleanup as prep work.
Sure, for step two, I'll read, confirm and try to remove these dummy
functions.
> > ... are these all the callbacks we need to take care of? What about
> > other console drivers that don't register these callbacks? ...
> >
> > ... for example, mdacon.c? What font does mdacon.c use? I know that
> > /lib/fonts/ exports two functions, find_font() and get_default_font(),
> > but I don't see them being used in mdacon.c.
>
> I think all other consoles either don't have fonts at all, or only
> support built-in fonts.
Ah, I see. I'll search for find_font() and get_default_font() when
dealing with built-in fonts, then. These files are using them, in
addition to fbcon.c:
drivers/firmware/efi/earlycon.c: font = get_default_font(xres, yres, -1, -1);
drivers/video/console/sticore.c: fbfont = get_default_font(1024,768, ~(u32)0, ~(u32)0);
drivers/media/pci/solo6x10/solo6x10-enc.c: const struct font_desc *vga = find_font("VGA8x16");
drivers/media/test-drivers/vimc/vimc-core.c: const struct font_desc *font = find_font("VGA8x16");
drivers/media/test-drivers/vivid/vivid-core.c: const struct font_desc *font = find_font("VGA8x16");
drivers/usb/misc/sisusbvga/sisusb.c: myfont = find_font("VGA8x16");
drivers/video/console/sticore.c: fbfont = find_font(fbfont_name);
> > Ah, and speaking of built-in fonts, see fbcon_startup():
> >
> > /* Setup default font */
> > [...]
> > vc->vc_font.charcount = 256; /* FIXME Need to support more fonts */
> > ^^^^^^^^^^^^^^^
> >
> > This is because find_font() and get_default_font() return a `struct
> > font_desc *`, but `struct font_desc` doesn't contain `charcount`. I
> > think we also need to add a `charcount` field to `struct font_desc`.
>
> Hm yeah ... I guess maybe struct font_desc should be the starting
> point for the kernel internal font structure. It's at least there
> already ...
I see, that will also make handling built-in fonts much easier!
> > Currently `struct vc_data` contains a `struct console_font vc_font`, and
> > I think this is making gradual conversion very hard. As an example, in
> > fbcon_do_set_font(), we update `vc->vc_font`. We lose all the extra
> > information we want in `kernel_console_font`, as long as `struct
> > vc_data` still uses `console_font`...
> >
> > However, if we let `struct vc_data` use `kernel_console_font` instead,
> > we'll have to handle a lot of things in one go:
> >
> > $ vgrep --no-less --no-header ".vc_font" | wc -l
> > 296
> > $ echo ":("
> > :(
>
> Yes :-/
>
> This is essentially why the entire vc/fbcon layer is such a mess. It's
> a chaos, it doesn't really have clear abstraction, and very often the
> uapi structures (see also conf_font_op) leak deeply into the
> implementation, which means changing anything is nearly impossible ...
>
> I think for vc_date->vc_font we might need a multi-step approach:
> - first add a new helper function which sets the font for a vc using
> an uapi console_font struct (and probably hard-coded assumes cnt ==
> 256.
But user fonts may have a charcount different to 256... But yes I'll try
to figure out how.
> - roll that out everwhere
> - change the type of vc_font to what we want (which should only need a
> change in the helper function, which will also set charcount hopefully
> correctly, using the hard-coded assumption
> - have another functions which sets the vf_font using a
> kernel_console_font for all the cases where it matters
> - now you can start using it and assume the charcount is set correctly
>
> It's a journey unfortunately.
But at least it now sounds manageable! :)
Thank you, I'll look into this (especially the user charcount issue
mentioned above) after cleaning up the uAPi structs and dummy functions.
> > The good news is, I've tried cleaning up all the macros in fbcon.c in my
> > playground, and things seem to work. For example, currently we have:
> >
> > if (userfont)
> > cnt = FNTCHARCNT(data);
> > else
> > cnt = 256;
> >
> > After introducing `kernel_console_font` (and adding `charcount` to
> > `struct font_desc` etc.), this should look like:
> >
> > #define to_font(_data) container_of(_data, struct kernel_console_font, data)
> > [...]
> > cnt = to_font(data)->charcount;
>
> Hm I guess we can't unify font_desc and the kernel_console_font we're
> talking about into one? I think that was brough up already somewhere
> else in this thread ...
Sure, let us use `font_desc` from now on.
> > No more `if` and `else`, and the framebuffer layer will be able to
> > support new bulit-in fonts that have more than 256 characters. This
> > seems really nice, so I'd like to spend some time working on it.
> >
> > However before I start working on real patches, do you have suggestions
> > about which console driver I should start with, or how should I split up
> > the work in general? I couldn't think of how do we clean up subsystems
> > one by one, while keeping a `console_font` in `struct vc_data`.
>
> I think from a "stop security bugs" trying to clean up fbcon is the
> important part. That's also the most complex (only one that supports
> the default and copy functions it seems, and also one of the few that
> supports get). The other ones I think we should just try to not break.
> vgacon should still be useable (but I think only on systems where you
> can boot into legacy bios, not into uefi, at least on x86). I have no
> idea where some of the other consoles are even used.
>
> For first steps I'd start with demidlayering some of the internal
> users of uapi structs, like the console_font_op really shouldn't be
> used anywhere in any function, except in the ioctl handler that
> converts it into the right function call. You'll probably discover a
> few other places like this on the go.
Sure, I'll start from this, then cleaning up these dummy functions, then
`vc_data`. Thank you for the insights!
Peilin Ye
More information about the dri-devel
mailing list