Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Wed, Sep 30, 2020 at 01:52:11PM +0200, Greg Kroah-Hartman wrote: > On Wed, Sep 30, 2020 at 01:25:14PM +0200, Daniel Vetter wrote: > > On Wed, Sep 30, 2020 at 12:56 PM Peilin Ye wrote: > > > Yes, and built-in fonts don't use refcount. Or maybe we can let > > > find_font() and get_default_font() kmalloc() a copy of built-in font > > > data, then keep track of refcount for both user and built-in fonts, but > > > that will waste a few K of memory for each built-in font we use... > > > > A possible trick for this would be to make sure built-in fonts start > > out with a refcount of 1. So never get freed. Plus maybe a check that > > if the name is set, then it's a built-in font and if we ever underflow > > the refcount we just WARN, but don't free anything. > > > > Another trick would be kern_font_get/put wrappers (we'd want those > > anyway if the userspace fonts are refcounted) and if kern_font->name > > != NULL (i.e. built-in font with name) then we simply don't call > > kref_get/put. > > Ick, don't do that, the first trick of having them start out with an > increased reference count is the best way here. Makes the code simpler > and no special cases for the tear-down path. I see, I'll just let them start out with 1, and only check `->name != NULL` in kern_font_put(). Thank you! Peilin Ye
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Wed, Sep 30, 2020 at 01:25:14PM +0200, Daniel Vetter wrote: > On Wed, Sep 30, 2020 at 12:56 PM Peilin Ye wrote: > > > > On Wed, Sep 30, 2020 at 11:53:17AM +0200, Daniel Vetter wrote: > > > On Wed, Sep 30, 2020 at 03:11:51AM -0400, Peilin Ye wrote: > > > > On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote: > > > > > On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye > > > > > wrote: > > > > > > 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! > > > > > > I think the only downside with starting with font_desc as the internal > > > font represenation is that there's a few fields we don't need/have for > > > userspace fonts (like the id/name stuff). So any helpers to e.g. print out > > > font information need to make sure they don't trip over that > > > > > > But otherwise I don't see a problem with this, I think. > > > > Yes, and built-in fonts don't use refcount. Or maybe we can let > > find_font() and get_default_font() kmalloc() a copy of built-in font > > data, then keep track of refcount for both user and built-in fonts, but > > that will waste a few K of memory for each built-in font we use... > > A possible trick for this would be to make sure built-in fonts start > out with a refcount of 1. So never get freed. Plus maybe a check that > if the name is set, then it's a built-in font and if we ever underflow > the refcount we just WARN, but don't free anything. > > Another trick would be kern_font_get/put wrappers (we'd want those > anyway if the userspace fonts are refcounted) and if kern_font->name > != NULL (i.e. built-in font with name) then we simply don't call > kref_get/put. Ick, don't do that, the first trick of having them start out with an increased reference count is the best way here. Makes the code simpler and no special cases for the tear-down path. thanks, greg k-h
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Wed, Sep 30, 2020 at 12:56 PM Peilin Ye wrote: > > On Wed, Sep 30, 2020 at 11:53:17AM +0200, Daniel Vetter wrote: > > On Wed, Sep 30, 2020 at 03:11:51AM -0400, Peilin Ye wrote: > > > On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote: > > > > On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye wrote: > > > > > 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! > > > > I think the only downside with starting with font_desc as the internal > > font represenation is that there's a few fields we don't need/have for > > userspace fonts (like the id/name stuff). So any helpers to e.g. print out > > font information need to make sure they don't trip over that > > > > But otherwise I don't see a problem with this, I think. > > Yes, and built-in fonts don't use refcount. Or maybe we can let > find_font() and get_default_font() kmalloc() a copy of built-in font > data, then keep track of refcount for both user and built-in fonts, but > that will waste a few K of memory for each built-in font we use... A possible trick for this would be to make sure built-in fonts start out with a refcount of 1. So never get freed. Plus maybe a check that if the name is set, then it's a built-in font and if we ever underflow the refcount we just WARN, but don't free anything. Another trick would be kern_font_get/put wrappers (we'd want those anyway if the userspace fonts are refcounted) and if kern_font->name != NULL (i.e. built-in font with name) then we simply don't call kref_get/put. -Daniel > > > > 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. > > > > Hm yeah, maybe we need a helper to give us the charcount then, which by > > default is using the magic negative offset. > > Ah, I see! :) > > > Then once we've converted everything over to explicitly passing charcount > > around, we can switch that helper. So something like > > > > int kern_font_charcount(struct kern_font *font); > > > > Feel free to bikeshed the struct name however you see fit :-) > > I think both `kern_font` and `font_desc` makes sense, naming is so > hard... > > > > > 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! > > > > Please don't take this rough plan as fixed, it's just where I'd start from > > browsing the code and your analysis a bit. We'll probably have to adapt as > > we go and more nasty things turn up ... > > Sure, I'll first give it a try and see. Thank you! > > Peilin Ye > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Wed, Sep 30, 2020 at 11:53:17AM +0200, Daniel Vetter wrote: > On Wed, Sep 30, 2020 at 03:11:51AM -0400, Peilin Ye wrote: > > On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote: > > > On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye wrote: > > > > 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! > > I think the only downside with starting with font_desc as the internal > font represenation is that there's a few fields we don't need/have for > userspace fonts (like the id/name stuff). So any helpers to e.g. print out > font information need to make sure they don't trip over that > > But otherwise I don't see a problem with this, I think. Yes, and built-in fonts don't use refcount. Or maybe we can let find_font() and get_default_font() kmalloc() a copy of built-in font data, then keep track of refcount for both user and built-in fonts, but that will waste a few K of memory for each built-in font we use... > > > 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. > > Hm yeah, maybe we need a helper to give us the charcount then, which by > default is using the magic negative offset. Ah, I see! :) > Then once we've converted everything over to explicitly passing charcount > around, we can switch that helper. So something like > > int kern_font_charcount(struct kern_font *font); > > Feel free to bikeshed the struct name however you see fit :-) I think both `kern_font` and `font_desc` makes sense, naming is so hard... > > > 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! > > Please don't take this rough plan as fixed, it's just where I'd start from > browsing the code and your analysis a bit. We'll probably have to adapt as > we go and more nasty things turn up ... Sure, I'll first give it a try and see. Thank you! Peilin Ye
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Wed, Sep 30, 2020 at 03:11:51AM -0400, Peilin Ye wrote:
> On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote:
> > On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye 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 FileLine 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.c3121 .con_font_get
> > > = fbcon_get_font,
> > > $
> > > $ vgrep "\.con_font_default"
> > > Index FileLine 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.c3122 .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 FileLine 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.c3123 .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
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Wed, Sep 30, 2020 at 07:26:52AM +0200, Jiri Slaby wrote: > On 29. 09. 20, 14:34, Peilin Ye wrote: > > 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`. > > Hi, > > feel free to change struct vc_data's content as you need, of course. > Only the UAPI _definitions_ have to be preserved (like struct console_font). Ah, I see. Thank you for the reminder! Peilin Ye
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Tue, Sep 29, 2020 at 04:38:49PM +0200, Daniel Vetter wrote:
> On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye 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 FileLine 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.c3121 .con_font_get
> > = fbcon_get_font,
> > $
> > $ vgrep "\.con_font_default"
> > Index FileLine 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.c3122 .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 FileLine 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.c3123 .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
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On 29. 09. 20, 14:34, Peilin Ye wrote: > 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`. Hi, feel free to change struct vc_data's content as you need, of course. Only the UAPI _definitions_ have to be preserved (like struct console_font). thanks, -- js suse labs
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Tue, Sep 29, 2020 at 2:34 PM Peilin Ye wrote:
>
> On Fri, Sep 25, 2020 at 03:25:51PM +0200, Daniel Vetter wrote:
> > I think the only way to make this work is that we have one place which
> > takes in the userspace uapi struct, and then converts it once into a
> > kernel_console_font. With all the error checking.
>
> Hi Daniel,
>
> 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.
> 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.
> Index FileLine Content
> 0 drivers/usb/misc/sisusbvga/sisusb_con.c 1294 .con_font_set =
> sisusbcon_font_set,
> 1 drivers/usb/misc/sisusbvga/sisusb_con.c 1378 .con_font_set =
> sisusbdummycon_font_set,
> 2 drivers/video/console/dummycon.c 162 .con_font_set =
> dummycon_font_set,
> 3 drivers/video/console/newport_con.c 693 .con_font_set =
> newport_font_set,
> 4 drivers/video/console/vgacon.c 1226 .con_font_set =
> vgacon_font_set,
> 5 drivers/video/fbdev/core/fbcon.c3120 .con_font_set
> = fbcon_set_font,
> $
> $ vgrep "\.con_font_get"
> Index FileLine 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.c3121 .con_font_get
> = fbcon_get_font,
> $
> $ vgrep "\.con_font_default"
> Index FileLine 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.c3122 .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 FileLine 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.c3123 .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.
> $ _
>
> ... are
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Fri, Sep 25, 2020 at 03:25:51PM +0200, Daniel Vetter wrote:
> I think the only way to make this work is that we have one place which
> takes in the userspace uapi struct, and then converts it once into a
> kernel_console_font. With all the error checking.
Hi Daniel,
It seems that users don't use `console_font` directly, they use
`console_font_op`. Then, in TTY:
(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;
}
These 4 functions allocate `console_font`. We can replace them with our
`kernel_console_font`. So, ...
$ vgrep "\.con_font_set"
Index FileLine Content
0 drivers/usb/misc/sisusbvga/sisusb_con.c 1294 .con_font_set =
sisusbcon_font_set,
1 drivers/usb/misc/sisusbvga/sisusb_con.c 1378 .con_font_set =
sisusbdummycon_font_set,
2 drivers/video/console/dummycon.c 162 .con_font_set =
dummycon_font_set,
3 drivers/video/console/newport_con.c 693 .con_font_set =
newport_font_set,
4 drivers/video/console/vgacon.c 1226 .con_font_set =
vgacon_font_set,
5 drivers/video/fbdev/core/fbcon.c3120 .con_font_set
= fbcon_set_font,
$
$ vgrep "\.con_font_get"
Index FileLine 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.c3121 .con_font_get
= fbcon_get_font,
$
$ vgrep "\.con_font_default"
Index FileLine 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,
2 drivers/video/console/newport_con.c 694 .con_font_default =
newport_font_default,
3 drivers/video/fbdev/core/fbcon.c3122 .con_font_default=
fbcon_set_def_font,
$
$ vgrep "\.con_font_copy"
Index FileLine 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,
2 drivers/video/fbdev/core/fbcon.c3123 .con_font_copy
= fbcon_copy_font,
$ _
... are these all the callbacks we need to take care of? What about
other console drivers that don't register these callbacks? ...
$ vgrep "\.con_init"
Index FileLine Content
[...]
3 drivers/usb/misc/sisusbvga/sisusb_con.c 1285 .con_init =
sisusbcon_init,
4 drivers/usb/misc/sisusbvga/sisusb_con.c 1369 .con_init =
sisusbdummycon_init,
5 drivers/video/console/dummycon.c 153 .con_init =
dummycon_init,
6 drivers/video/console/mdacon.c 544 .con_init =
mdacon_init,
7 drivers/video/console/newport_con.c 684 .con_init =
newport_init,
8 drivers/video/console/sticon.c 328 .con_init=
sticon_init,
9 drivers/video/console/vgacon.c 1217 .con_init = vgacon_init,
10 drivers/video/fbdev/core/fbcon.c3111 .con_init =
fbcon_init,
[...]
... 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.
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`.
> Then all internal code deals in terms of kernel_console_font, with
> properly typed and named struct members and helper functions and
> everything. And we might need a gradual conversion for this, so that first
> we can convert over invidual console drivers, then subsystems, until at
> the end we've pushed the conversion from uapi array to kernel_console_font
> all the way to the ioctl entry points.
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
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Tue, Sep 29, 2020 at 11:09:45AM +0200, Daniel Vetter wrote: > If you want to follow along a bit I think would be good to subscribe to > the dri-devel mailing list. At least for all the fbcon/fbdev/gpu stuff. > > I don't think there's a dedicated list for vt/console stuff, aside from > Greg's inbox :-) Ah, I've been checking lore.kernel.org/dri-devel/ once a while. Sure! I'll subscribe right now :) Peilin Ye
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Fri, Sep 25, 2020 at 11:35:09AM -0400, Peilin Ye wrote: > On Fri, Sep 25, 2020 at 03:25:51PM +0200, Daniel Vetter wrote: > > I think the only way to make this work is that we have one place which > > takes in the userspace uapi struct, and then converts it once into a > > kernel_console_font. With all the error checking. > > Ah, I didn't think of that! When trying to introduce > `kernel_console_font` I ended up using the uapi version and the kernel > version in parallel... > > > Then all internal code deals in terms of kernel_console_font, with > > properly typed and named struct members and helper functions and > > everything. And we might need a gradual conversion for this, so that first > > we can convert over invidual console drivers, then subsystems, until at > > the end we've pushed the conversion from uapi array to kernel_console_font > > all the way to the ioctl entry points. > > > > But that's indeed a huge pile of work, and fair warning: fbcon is > > semi-orphaned, so by doing this you'll pretty much volunteer for > > maintainership :-) > > > > But I'd be very happy to help get this done and throw some maintainership > > credentials at you in the proces ... > > Sounds exciting, I will be glad to do this! I'm just a beginner, but I > will try to do what I can do. If you want to follow along a bit I think would be good to subscribe to the dri-devel mailing list. At least for all the fbcon/fbdev/gpu stuff. I don't think there's a dedicated list for vt/console stuff, aside from Greg's inbox :-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Fri, Sep 25, 2020 at 03:25:51PM +0200, Daniel Vetter wrote: > I think the only way to make this work is that we have one place which > takes in the userspace uapi struct, and then converts it once into a > kernel_console_font. With all the error checking. Ah, I didn't think of that! When trying to introduce `kernel_console_font` I ended up using the uapi version and the kernel version in parallel... > Then all internal code deals in terms of kernel_console_font, with > properly typed and named struct members and helper functions and > everything. And we might need a gradual conversion for this, so that first > we can convert over invidual console drivers, then subsystems, until at > the end we've pushed the conversion from uapi array to kernel_console_font > all the way to the ioctl entry points. > > But that's indeed a huge pile of work, and fair warning: fbcon is > semi-orphaned, so by doing this you'll pretty much volunteer for > maintainership :-) > > But I'd be very happy to help get this done and throw some maintainership > credentials at you in the proces ... Sounds exciting, I will be glad to do this! I'm just a beginner, but I will try to do what I can do. Thank you, Peilin Ye
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Fri, Sep 25, 2020 at 06:13:00AM -0400, Peilin Ye wrote:
> Hi all!
>
> On Fri, Sep 25, 2020 at 08:46:04AM +0200, Jiri Slaby wrote:
> > > In order to perform a reliable range check, fbcon_get_font() needs to know
> > > `FONTDATAMAX` for each built-in font under lib/fonts/. Unfortunately, we
> > > do not keep that information in our font descriptor,
> > > `struct console_font`:
> > >
> > > (include/uapi/linux/kd.h)
> > > struct console_font {
> > > unsigned int width, height; /* font size */
> > > unsigned int charcount;
> > > unsigned char *data;/* font data with height fixed to 32 */
> > > };
> > >
> > > To make things worse, `struct console_font` is part of the UAPI, so we
> > > cannot add a new field to keep track of `FONTDATAMAX`.
> >
> > Hi,
> >
> > but you still can define struct kernel_console_font containing struct
> > console_font and the 4 more members you need in the kernel. See below.
> >
> > > Fortunately, the framebuffer layer itself gives us a hint of how to
> > > resolve this issue without changing UAPI. When allocating a buffer for a
> > > user-provided font, fbcon_set_font() reserves four "extra words" at the
> > > beginning of the buffer:
> > >
> > > (drivers/video/fbdev/core/fbcon.c)
> > > new_data = kmalloc(FONT_EXTRA_WORDS * sizeof(int) + size, GFP_USER);
> >
> > I might be missing something (like coffee in the morning), but why don't
> > you just:
> > 1) declare struct font_data as
> > {
> > unsigned sum, char_count, size, refcnt;
> > const unsigned char data[];
> > }
> >
> > Or maybe "struct console_font font" instead of "const unsigned char
> > data[]", if need be.
> >
> > 2) allocate by:
> > kmalloc(struct_size(struct font_data, data, size));
> >
> > 3) use container_of wherever needed
> >
> > That is you name the data on negative indexes using struct as you
> > already have to define one.
> >
> > Then you don't need the ugly macros with negative indexes. And you can
> > pass this structure down e.g. to fbcon_do_set_font, avoiding potential
> > mistakes in accessing data[-1] and similar.
>
> Sorry that I didn't mention it in the cover letter, but yes, I've tried
> this - a new `kernel_console_font` would be much cleaner than negative
> array indexing.
>
> The reason I ended up giving it up was, frankly speaking, these macros
> are being used at about 30 places, and I am not familiar enough with the
> framebuffer and newport_con code, so I wasn't confident how to clean
> them up and plug in `kernel_console_font` properly...
>
> Another reason was that, functions like fbcon_get_font() handle both user
> fonts and built-in fonts, so I wanted a single solution for both of
> them. I think we can't really introduce `kernel_console_font` while
> keeping these macros, that would make the error handling logics etc.
> very messy.
>
> I'm not very sure what to do now. Should I give it another try cleaning
> up all the macros?
>
> And thank you for reviewing this!
I think the only way to make this work is that we have one place which
takes in the userspace uapi struct, and then converts it once into a
kernel_console_font. With all the error checking.
Then all internal code deals in terms of kernel_console_font, with
properly typed and named struct members and helper functions and
everything. And we might need a gradual conversion for this, so that first
we can convert over invidual console drivers, then subsystems, until at
the end we've pushed the conversion from uapi array to kernel_console_font
all the way to the ioctl entry points.
But that's indeed a huge pile of work, and fair warning: fbcon is
semi-orphaned, so by doing this you'll pretty much volunteer for
maintainership :-)
But I'd be very happy to help get this done and throw some maintainership
credentials at you in the proces ...
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
Hi all!
On Fri, Sep 25, 2020 at 08:46:04AM +0200, Jiri Slaby wrote:
> > In order to perform a reliable range check, fbcon_get_font() needs to know
> > `FONTDATAMAX` for each built-in font under lib/fonts/. Unfortunately, we
> > do not keep that information in our font descriptor,
> > `struct console_font`:
> >
> > (include/uapi/linux/kd.h)
> > struct console_font {
> > unsigned int width, height; /* font size */
> > unsigned int charcount;
> > unsigned char *data;/* font data with height fixed to 32 */
> > };
> >
> > To make things worse, `struct console_font` is part of the UAPI, so we
> > cannot add a new field to keep track of `FONTDATAMAX`.
>
> Hi,
>
> but you still can define struct kernel_console_font containing struct
> console_font and the 4 more members you need in the kernel. See below.
>
> > Fortunately, the framebuffer layer itself gives us a hint of how to
> > resolve this issue without changing UAPI. When allocating a buffer for a
> > user-provided font, fbcon_set_font() reserves four "extra words" at the
> > beginning of the buffer:
> >
> > (drivers/video/fbdev/core/fbcon.c)
> > new_data = kmalloc(FONT_EXTRA_WORDS * sizeof(int) + size, GFP_USER);
>
> I might be missing something (like coffee in the morning), but why don't
> you just:
> 1) declare struct font_data as
> {
> unsigned sum, char_count, size, refcnt;
> const unsigned char data[];
> }
>
> Or maybe "struct console_font font" instead of "const unsigned char
> data[]", if need be.
>
> 2) allocate by:
> kmalloc(struct_size(struct font_data, data, size));
>
> 3) use container_of wherever needed
>
> That is you name the data on negative indexes using struct as you
> already have to define one.
>
> Then you don't need the ugly macros with negative indexes. And you can
> pass this structure down e.g. to fbcon_do_set_font, avoiding potential
> mistakes in accessing data[-1] and similar.
Sorry that I didn't mention it in the cover letter, but yes, I've tried
this - a new `kernel_console_font` would be much cleaner than negative
array indexing.
The reason I ended up giving it up was, frankly speaking, these macros
are being used at about 30 places, and I am not familiar enough with the
framebuffer and newport_con code, so I wasn't confident how to clean
them up and plug in `kernel_console_font` properly...
Another reason was that, functions like fbcon_get_font() handle both user
fonts and built-in fonts, so I wanted a single solution for both of
them. I think we can't really introduce `kernel_console_font` while
keeping these macros, that would make the error handling logics etc.
very messy.
I'm not very sure what to do now. Should I give it another try cleaning
up all the macros?
And thank you for reviewing this!
Peilin Ye
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Thu, Sep 24, 2020 at 04:09:37PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 24, 2020 at 09:38:22AM -0400, Peilin Ye wrote:
> > Hi all,
> >
> > syzbot has reported [1] a global out-of-bounds read issue in
> > fbcon_get_font(). A malicious user may resize `vc_font.height` to a large
> > value in vt_ioctl(), causing fbcon_get_font() to overflow our built-in
> > font data buffers, declared in lib/fonts/font_*.c:
> >
> > (e.g. lib/fonts/font_8x8.c)
> > #define FONTDATAMAX 2048
> >
> > static const unsigned char fontdata_8x8[FONTDATAMAX] = {
> >
> > /* 0 0x00 '^@' */
> > 0x00, /* */
> > 0x00, /* */
> > 0x00, /* */
> > 0x00, /* */
> > 0x00, /* */
> > 0x00, /* */
> > 0x00, /* */
> > 0x00, /* */
> > [...]
> >
> > In order to perform a reliable range check, fbcon_get_font() needs to know
> > `FONTDATAMAX` for each built-in font under lib/fonts/. Unfortunately, we
> > do not keep that information in our font descriptor,
> > `struct console_font`:
> >
> > (include/uapi/linux/kd.h)
> > struct console_font {
> > unsigned int width, height; /* font size */
> > unsigned int charcount;
> > unsigned char *data;/* font data with height fixed to 32 */
> > };
> >
> > To make things worse, `struct console_font` is part of the UAPI, so we
> > cannot add a new field to keep track of `FONTDATAMAX`.
> >
> > Fortunately, the framebuffer layer itself gives us a hint of how to
> > resolve this issue without changing UAPI. When allocating a buffer for a
> > user-provided font, fbcon_set_font() reserves four "extra words" at the
> > beginning of the buffer:
> >
> > (drivers/video/fbdev/core/fbcon.c)
> > new_data = kmalloc(FONT_EXTRA_WORDS * sizeof(int) + size, GFP_USER);
> > [...]
> > new_data += FONT_EXTRA_WORDS * sizeof(int);
> > FNTSIZE(new_data) = size;
> > FNTCHARCNT(new_data) = charcount;
> > REFCOUNT(new_data) = 0; /* usage counter */
> > [...]
> > FNTSUM(new_data) = csum;
> >
> > Later, to get the size of a data buffer, the framebuffer layer simply
> > calls FNTSIZE() on it:
> >
> > (drivers/video/fbdev/core/fbcon.h)
> > /* Font */
> > #define REFCOUNT(fd)(((int *)(fd))[-1])
> > #define FNTSIZE(fd) (((int *)(fd))[-2])
> > #define FNTCHARCNT(fd) (((int *)(fd))[-3])
> > #define FNTSUM(fd) (((int *)(fd))[-4])
> > #define FONT_EXTRA_WORDS 4
> >
> > Currently, this is only done for user-provided fonts. Let us do the same
> > thing for built-in fonts, prepend these "extra words" (including
> > `FONTDATAMAX`) to their data buffers, so that other subsystems, like the
> > framebuffer layer, can use these macros on all fonts, no matter built-in
> > or user-provided. As an example, this series fixes the syzbot issue in
> > fbcon_get_font():
> >
> > (drivers/video/fbdev/core/fbcon.c)
> > if (font->width <= 8) {
> > j = vc->vc_font.height;
> > + if (font->charcount * j > FNTSIZE(fontdata))
> > + return -EINVAL;
> > [...]
> >
> > Similarly, newport_con also use these macros. It only uses three of them:
> >
> > (drivers/video/console/newport_con.c)
> > /* borrowed from fbcon.c */
> > #define REFCOUNT(fd)(((int *)(fd))[-1])
> > #define FNTSIZE(fd) (((int *)(fd))[-2])
> > #define FNTCHARCNT(fd) (((int *)(fd))[-3])
> > #define FONT_EXTRA_WORDS 3
> >
> > To keep things simple, move all these macro definitions to ,
> > use four words instead of three, and initialize the fourth word in
> > newport_set_font() properly.
> >
> > Many thanks to Greg Kroah-Hartman , who
> > reviewed and improved this series!
> >
> > [1]: KASAN: global-out-of-bounds Read in fbcon_get_font
> >
> > https://syzkaller.appspot.com/bug?id=08b8be45afea11888776f897895aef9ad1c3ecfd
> >
> > Peilin Ye (3):
> > fbdev, newport_con: Move FONT_EXTRA_WORDS macros into linux/font.h
> > Fonts: Support FONT_EXTRA_WORDS macros for built-in fonts
> > fbcon: Fix global-out-of-bounds read in fbcon_get_font()
> >
> > drivers/video/console/newport_con.c | 7 +--
> > drivers/video/fbdev/core/fbcon.c| 12
> > drivers/video/fbdev/core/fbcon.h| 7 ---
> > drivers/video/fbdev/core/fbcon_rotate.c | 1 +
> > drivers/video/fbdev/core/tileblit.c | 1 +
> > include/linux/font.h| 13 +
> > lib/fonts/font_10x18.c | 9 -
> > lib/fonts/font_6x10.c | 9 +
> > lib/fonts/font_6x11.c | 9 -
> > lib/fonts/font_7x14.c | 9 -
> > lib/fonts/font_8x16.c | 9 -
> > lib/fonts/font_8x8.c| 9 -
> > lib/fonts/font_acorn_8x8.c | 9 ++---
> > lib/fonts/font_mini_4x6.c | 8
> >
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On 24. 09. 20, 15:38, Peilin Ye wrote:
> Hi all,
>
> syzbot has reported [1] a global out-of-bounds read issue in
> fbcon_get_font(). A malicious user may resize `vc_font.height` to a large
> value in vt_ioctl(), causing fbcon_get_font() to overflow our built-in
> font data buffers, declared in lib/fonts/font_*.c:
>
> (e.g. lib/fonts/font_8x8.c)
> #define FONTDATAMAX 2048
>
> static const unsigned char fontdata_8x8[FONTDATAMAX] = {
>
> /* 0 0x00 '^@' */
> 0x00, /* */
> 0x00, /* */
> 0x00, /* */
> 0x00, /* */
> 0x00, /* */
> 0x00, /* */
> 0x00, /* */
> 0x00, /* */
> [...]
>
> In order to perform a reliable range check, fbcon_get_font() needs to know
> `FONTDATAMAX` for each built-in font under lib/fonts/. Unfortunately, we
> do not keep that information in our font descriptor,
> `struct console_font`:
>
> (include/uapi/linux/kd.h)
> struct console_font {
> unsigned int width, height; /* font size */
> unsigned int charcount;
> unsigned char *data;/* font data with height fixed to 32 */
> };
>
> To make things worse, `struct console_font` is part of the UAPI, so we
> cannot add a new field to keep track of `FONTDATAMAX`.
Hi,
but you still can define struct kernel_console_font containing struct
console_font and the 4 more members you need in the kernel. See below.
> Fortunately, the framebuffer layer itself gives us a hint of how to
> resolve this issue without changing UAPI. When allocating a buffer for a
> user-provided font, fbcon_set_font() reserves four "extra words" at the
> beginning of the buffer:
>
> (drivers/video/fbdev/core/fbcon.c)
> new_data = kmalloc(FONT_EXTRA_WORDS * sizeof(int) + size, GFP_USER);
I might be missing something (like coffee in the morning), but why don't
you just:
1) declare struct font_data as
{
unsigned sum, char_count, size, refcnt;
const unsigned char data[];
}
Or maybe "struct console_font font" instead of "const unsigned char
data[]", if need be.
2) allocate by:
kmalloc(struct_size(struct font_data, data, size));
3) use container_of wherever needed
That is you name the data on negative indexes using struct as you
already have to define one.
Then you don't need the ugly macros with negative indexes. And you can
pass this structure down e.g. to fbcon_do_set_font, avoiding potential
mistakes in accessing data[-1] and similar.
thanks,
--
js
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Thu, Sep 24, 2020 at 06:45:16PM +0300, Dan Carpenter wrote: > Smatch has a tool to show where struct members are set. > > `~/smatch/smatch_data/db/smdb.py where console_font height` > > It's not perfect and this output comes from allmodconfig on yesterday's > linux-next. > > regards, > dan carpenter Wow, thank you for the really interesting information! > drivers/video/console/vgacon.c | vgacon_init| (struct > console_font)->height | 0-32 > drivers/video/console/vgacon.c | vgacon_adjust_height | (struct > console_font)->height | 1-32 > drivers/video/fbdev/core/fbcon.c | fbcon_startup | (struct > console_font)->height | 6,8,10-11,14,16,18,22,32 > drivers/video/fbdev/core/fbcon.c | fbcon_init | (struct > console_font)->height | 6,8,10-11,14,16,18,22,32 > drivers/video/fbdev/core/fbcon.c | fbcon_do_set_font | (struct > console_font)->height | 0-u32max > drivers/video/fbdev/core/fbcon.c | fbcon_set_def_font | (struct > console_font)->height | 6,8,10-11,14,16,18,22,32 > drivers/usb/misc/sisusbvga/sisusb_con.c | sisusbcon_init | > (struct console_font)->height | 0-u32max In looking at this one, c->vc_font.height = sisusb->current_font_height; `sisusb->current_font_height` is only set in sisusbcon_do_font_op(): sisusb->current_font_height = fh; and... > drivers/usb/misc/sisusbvga/sisusb_con.c | sisusbcon_do_font_op | > (struct console_font)->height | 1-32 ...sisusbcon_do_font_op() is called in four places, with an `fh` of either 16, `sisusb->font_backup_height`, or `font->height`. The latter two cases all come from sisusbcon_font_set(), whose dispatcher, con_font_set() gurantees that `font->height` is less than or equal to 32, as shown by Smatch here. > drivers/tty/vt/vt_ioctl.c | vt_k_ioctl | (struct > console_font)->height | ignore > drivers/tty/vt/vt_ioctl.c | vt_resizex | (struct > console_font)->height | 0-u32max > drivers/tty/vt/vt_ioctl.c | vt_ioctl | (struct > console_font)->height | ignore > drivers/tty/vt/vt_ioctl.c | vt_compat_ioctl| (struct > console_font)->height | ignore > drivers/tty/vt/vt.c| vc_allocate| (struct > console_font)->height | 0 > drivers/tty/vt/vt.c| vt_resize | (struct > console_font)->height | ignore > drivers/tty/vt/vt.c| do_con_write | (struct > console_font)->height | ignore > drivers/tty/vt/vt.c| con_unthrottle | (struct > console_font)->height | ignore > drivers/tty/vt/vt.c| con_flush_chars| (struct > console_font)->height | ignore > drivers/tty/vt/vt.c| con_shutdown | (struct > console_font)->height | ignore > drivers/tty/vt/vt.c| con_cleanup| (struct > console_font)->height | ignore > drivers/tty/vt/vt.c| con_init | (struct > console_font)->height | 0 > drivers/tty/vt/vt.c| con_font_set | (struct > console_font)->height | 1-32 > drivers/tty/vt/vt.c| con_font_default | (struct > console_font)->height | 0-u32max > drivers/tty/vt/selection.c | paste_selection| (struct > console_font)->height | ignore I will go through the list starting from these "0-u32max" cases. Thanks again! Peilin Ye
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
Smatch has a tool to show where struct members are set. `~/smatch/smatch_data/db/smdb.py where console_font height` It's not perfect and this output comes from allmodconfig on yesterday's linux-next. regards, dan carpenter drivers/video/console/vgacon.c | vgacon_init| (struct console_font)->height | 0-32 drivers/video/console/vgacon.c | vgacon_adjust_height | (struct console_font)->height | 1-32 drivers/video/fbdev/core/fbcon.c | fbcon_startup | (struct console_font)->height | 6,8,10-11,14,16,18,22,32 drivers/video/fbdev/core/fbcon.c | fbcon_init | (struct console_font)->height | 6,8,10-11,14,16,18,22,32 drivers/video/fbdev/core/fbcon.c | fbcon_do_set_font | (struct console_font)->height | 0-u32max drivers/video/fbdev/core/fbcon.c | fbcon_set_def_font | (struct console_font)->height | 6,8,10-11,14,16,18,22,32 drivers/usb/misc/sisusbvga/sisusb_con.c | sisusbcon_init | (struct console_font)->height | 0-u32max drivers/usb/misc/sisusbvga/sisusb_con.c | sisusbcon_do_font_op | (struct console_font)->height | 1-32 drivers/tty/vt/vt_ioctl.c | vt_k_ioctl | (struct console_font)->height | ignore drivers/tty/vt/vt_ioctl.c | vt_resizex | (struct console_font)->height | 0-u32max drivers/tty/vt/vt_ioctl.c | vt_ioctl | (struct console_font)->height | ignore drivers/tty/vt/vt_ioctl.c | vt_compat_ioctl| (struct console_font)->height | ignore drivers/tty/vt/vt.c| vc_allocate| (struct console_font)->height | 0 drivers/tty/vt/vt.c| vt_resize | (struct console_font)->height | ignore drivers/tty/vt/vt.c| do_con_write | (struct console_font)->height | ignore drivers/tty/vt/vt.c| con_unthrottle | (struct console_font)->height | ignore drivers/tty/vt/vt.c| con_flush_chars| (struct console_font)->height | ignore drivers/tty/vt/vt.c| con_shutdown | (struct console_font)->height | ignore drivers/tty/vt/vt.c| con_cleanup| (struct console_font)->height | ignore drivers/tty/vt/vt.c| con_init | (struct console_font)->height | 0 drivers/tty/vt/vt.c| con_font_set | (struct console_font)->height | 1-32 drivers/tty/vt/vt.c| con_font_default | (struct console_font)->height | 0-u32max drivers/tty/vt/selection.c | paste_selection| (struct console_font)->height | ignore
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
Hi!
On Thu, Sep 24, 2020 at 02:42:18PM +, David Laight wrote:
> > On Thu, Sep 24, 2020 at 09:38:22AM -0400, Peilin Ye wrote:
> > > Hi all,
> > >
> > > syzbot has reported [1] a global out-of-bounds read issue in
> > > fbcon_get_font(). A malicious user may resize `vc_font.height` to a large
> > > value in vt_ioctl(), causing fbcon_get_font() to overflow our built-in
> > > font data buffers, declared in lib/fonts/font_*.c:
> ...
> > > (drivers/video/fbdev/core/fbcon.c)
> > > if (font->width <= 8) {
> > > j = vc->vc_font.height;
> > > + if (font->charcount * j > FNTSIZE(fontdata))
> > > + return -EINVAL;
>
> Can that still go wrong because the multiply wraps?
Thank you for bringing this up!
The resizing of `vc_font.height` happened in vt_resizex():
(drivers/tty/vt/vt_ioctl.c)
if (v.v_clin > 32)
return -EINVAL;
[...]
for (i = 0; i < MAX_NR_CONSOLES; i++) {
[...]
if (v.v_clin)
vcp->vc_font.height = v.v_clin;
^^
It does check if `v.v_clin` is greater than 32. And, currently, all
built-in fonts have a `charcount` of 256.
Therefore, for built-in fonts and resizing happened in vt_resizex(), it
cannot cause an interger overflow.
However I am not very sure about user-provided fonts, and if there are
other functions that can resize `height` or even `charcount` to a really
huge value, but I will do more investigation and think about it.
Thank you,
Peilin Ye
RE: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
> On Thu, Sep 24, 2020 at 09:38:22AM -0400, Peilin Ye wrote:
> > Hi all,
> >
> > syzbot has reported [1] a global out-of-bounds read issue in
> > fbcon_get_font(). A malicious user may resize `vc_font.height` to a large
> > value in vt_ioctl(), causing fbcon_get_font() to overflow our built-in
> > font data buffers, declared in lib/fonts/font_*.c:
...
> > (drivers/video/fbdev/core/fbcon.c)
> > if (font->width <= 8) {
> > j = vc->vc_font.height;
> > + if (font->charcount * j > FNTSIZE(fontdata))
> > + return -EINVAL;
Can that still go wrong because the multiply wraps?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Thu, Sep 24, 2020 at 04:09:37PM +0200, Greg Kroah-Hartman wrote: > On Thu, Sep 24, 2020 at 09:38:22AM -0400, Peilin Ye wrote: > > Peilin Ye (3): > > fbdev, newport_con: Move FONT_EXTRA_WORDS macros into linux/font.h > > Fonts: Support FONT_EXTRA_WORDS macros for built-in fonts > > fbcon: Fix global-out-of-bounds read in fbcon_get_font() > > > > drivers/video/console/newport_con.c | 7 +-- > > drivers/video/fbdev/core/fbcon.c| 12 > > drivers/video/fbdev/core/fbcon.h| 7 --- > > drivers/video/fbdev/core/fbcon_rotate.c | 1 + > > drivers/video/fbdev/core/tileblit.c | 1 + > > include/linux/font.h| 13 + > > lib/fonts/font_10x18.c | 9 - > > lib/fonts/font_6x10.c | 9 + > > lib/fonts/font_6x11.c | 9 - > > lib/fonts/font_7x14.c | 9 - > > lib/fonts/font_8x16.c | 9 - > > lib/fonts/font_8x8.c| 9 - > > lib/fonts/font_acorn_8x8.c | 9 ++--- > > lib/fonts/font_mini_4x6.c | 8 > > lib/fonts/font_pearl_8x8.c | 9 - > > lib/fonts/font_sun12x22.c | 9 - > > lib/fonts/font_sun8x16.c| 7 --- > > lib/fonts/font_ter16x32.c | 9 - > > 18 files changed, 79 insertions(+), 67 deletions(-) > > Gotta love going backwards in arrays :) > > Nice work, whole series is: > > Reviewed-by: Greg Kroah-Hartman Thank you for reviewing it! Peilin Ye
Re: [PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
On Thu, Sep 24, 2020 at 09:38:22AM -0400, Peilin Ye wrote:
> Hi all,
>
> syzbot has reported [1] a global out-of-bounds read issue in
> fbcon_get_font(). A malicious user may resize `vc_font.height` to a large
> value in vt_ioctl(), causing fbcon_get_font() to overflow our built-in
> font data buffers, declared in lib/fonts/font_*.c:
>
> (e.g. lib/fonts/font_8x8.c)
> #define FONTDATAMAX 2048
>
> static const unsigned char fontdata_8x8[FONTDATAMAX] = {
>
> /* 0 0x00 '^@' */
> 0x00, /* */
> 0x00, /* */
> 0x00, /* */
> 0x00, /* */
> 0x00, /* */
> 0x00, /* */
> 0x00, /* */
> 0x00, /* */
> [...]
>
> In order to perform a reliable range check, fbcon_get_font() needs to know
> `FONTDATAMAX` for each built-in font under lib/fonts/. Unfortunately, we
> do not keep that information in our font descriptor,
> `struct console_font`:
>
> (include/uapi/linux/kd.h)
> struct console_font {
> unsigned int width, height; /* font size */
> unsigned int charcount;
> unsigned char *data;/* font data with height fixed to 32 */
> };
>
> To make things worse, `struct console_font` is part of the UAPI, so we
> cannot add a new field to keep track of `FONTDATAMAX`.
>
> Fortunately, the framebuffer layer itself gives us a hint of how to
> resolve this issue without changing UAPI. When allocating a buffer for a
> user-provided font, fbcon_set_font() reserves four "extra words" at the
> beginning of the buffer:
>
> (drivers/video/fbdev/core/fbcon.c)
> new_data = kmalloc(FONT_EXTRA_WORDS * sizeof(int) + size, GFP_USER);
> [...]
> new_data += FONT_EXTRA_WORDS * sizeof(int);
> FNTSIZE(new_data) = size;
> FNTCHARCNT(new_data) = charcount;
> REFCOUNT(new_data) = 0; /* usage counter */
> [...]
> FNTSUM(new_data) = csum;
>
> Later, to get the size of a data buffer, the framebuffer layer simply
> calls FNTSIZE() on it:
>
> (drivers/video/fbdev/core/fbcon.h)
> /* Font */
> #define REFCOUNT(fd)(((int *)(fd))[-1])
> #define FNTSIZE(fd) (((int *)(fd))[-2])
> #define FNTCHARCNT(fd) (((int *)(fd))[-3])
> #define FNTSUM(fd) (((int *)(fd))[-4])
> #define FONT_EXTRA_WORDS 4
>
> Currently, this is only done for user-provided fonts. Let us do the same
> thing for built-in fonts, prepend these "extra words" (including
> `FONTDATAMAX`) to their data buffers, so that other subsystems, like the
> framebuffer layer, can use these macros on all fonts, no matter built-in
> or user-provided. As an example, this series fixes the syzbot issue in
> fbcon_get_font():
>
> (drivers/video/fbdev/core/fbcon.c)
> if (font->width <= 8) {
> j = vc->vc_font.height;
> + if (font->charcount * j > FNTSIZE(fontdata))
> + return -EINVAL;
> [...]
>
> Similarly, newport_con also use these macros. It only uses three of them:
>
> (drivers/video/console/newport_con.c)
> /* borrowed from fbcon.c */
> #define REFCOUNT(fd)(((int *)(fd))[-1])
> #define FNTSIZE(fd) (((int *)(fd))[-2])
> #define FNTCHARCNT(fd) (((int *)(fd))[-3])
> #define FONT_EXTRA_WORDS 3
>
> To keep things simple, move all these macro definitions to ,
> use four words instead of three, and initialize the fourth word in
> newport_set_font() properly.
>
> Many thanks to Greg Kroah-Hartman , who
> reviewed and improved this series!
>
> [1]: KASAN: global-out-of-bounds Read in fbcon_get_font
>
> https://syzkaller.appspot.com/bug?id=08b8be45afea11888776f897895aef9ad1c3ecfd
>
> Peilin Ye (3):
> fbdev, newport_con: Move FONT_EXTRA_WORDS macros into linux/font.h
> Fonts: Support FONT_EXTRA_WORDS macros for built-in fonts
> fbcon: Fix global-out-of-bounds read in fbcon_get_font()
>
> drivers/video/console/newport_con.c | 7 +--
> drivers/video/fbdev/core/fbcon.c| 12
> drivers/video/fbdev/core/fbcon.h| 7 ---
> drivers/video/fbdev/core/fbcon_rotate.c | 1 +
> drivers/video/fbdev/core/tileblit.c | 1 +
> include/linux/font.h| 13 +
> lib/fonts/font_10x18.c | 9 -
> lib/fonts/font_6x10.c | 9 +
> lib/fonts/font_6x11.c | 9 -
> lib/fonts/font_7x14.c | 9 -
> lib/fonts/font_8x16.c | 9 -
> lib/fonts/font_8x8.c| 9 -
> lib/fonts/font_acorn_8x8.c | 9 ++---
> lib/fonts/font_mini_4x6.c | 8
> lib/fonts/font_pearl_8x8.c | 9 -
> lib/fonts/font_sun12x22.c | 9 -
> lib/fonts/font_sun8x16.c| 7 ---
> lib/fonts/font_ter16x32.c | 9 -
> 18 files changed, 79
[PATCH 0/3] Prevent out-of-bounds access for built-in font data buffers
Hi all,
syzbot has reported [1] a global out-of-bounds read issue in
fbcon_get_font(). A malicious user may resize `vc_font.height` to a large
value in vt_ioctl(), causing fbcon_get_font() to overflow our built-in
font data buffers, declared in lib/fonts/font_*.c:
(e.g. lib/fonts/font_8x8.c)
#define FONTDATAMAX 2048
static const unsigned char fontdata_8x8[FONTDATAMAX] = {
/* 0 0x00 '^@' */
0x00, /* */
0x00, /* */
0x00, /* */
0x00, /* */
0x00, /* */
0x00, /* */
0x00, /* */
0x00, /* */
[...]
In order to perform a reliable range check, fbcon_get_font() needs to know
`FONTDATAMAX` for each built-in font under lib/fonts/. Unfortunately, we
do not keep that information in our font descriptor,
`struct console_font`:
(include/uapi/linux/kd.h)
struct console_font {
unsigned int width, height; /* font size */
unsigned int charcount;
unsigned char *data;/* font data with height fixed to 32 */
};
To make things worse, `struct console_font` is part of the UAPI, so we
cannot add a new field to keep track of `FONTDATAMAX`.
Fortunately, the framebuffer layer itself gives us a hint of how to
resolve this issue without changing UAPI. When allocating a buffer for a
user-provided font, fbcon_set_font() reserves four "extra words" at the
beginning of the buffer:
(drivers/video/fbdev/core/fbcon.c)
new_data = kmalloc(FONT_EXTRA_WORDS * sizeof(int) + size, GFP_USER);
[...]
new_data += FONT_EXTRA_WORDS * sizeof(int);
FNTSIZE(new_data) = size;
FNTCHARCNT(new_data) = charcount;
REFCOUNT(new_data) = 0; /* usage counter */
[...]
FNTSUM(new_data) = csum;
Later, to get the size of a data buffer, the framebuffer layer simply
calls FNTSIZE() on it:
(drivers/video/fbdev/core/fbcon.h)
/* Font */
#define REFCOUNT(fd)(((int *)(fd))[-1])
#define FNTSIZE(fd) (((int *)(fd))[-2])
#define FNTCHARCNT(fd) (((int *)(fd))[-3])
#define FNTSUM(fd) (((int *)(fd))[-4])
#define FONT_EXTRA_WORDS 4
Currently, this is only done for user-provided fonts. Let us do the same
thing for built-in fonts, prepend these "extra words" (including
`FONTDATAMAX`) to their data buffers, so that other subsystems, like the
framebuffer layer, can use these macros on all fonts, no matter built-in
or user-provided. As an example, this series fixes the syzbot issue in
fbcon_get_font():
(drivers/video/fbdev/core/fbcon.c)
if (font->width <= 8) {
j = vc->vc_font.height;
+ if (font->charcount * j > FNTSIZE(fontdata))
+ return -EINVAL;
[...]
Similarly, newport_con also use these macros. It only uses three of them:
(drivers/video/console/newport_con.c)
/* borrowed from fbcon.c */
#define REFCOUNT(fd)(((int *)(fd))[-1])
#define FNTSIZE(fd) (((int *)(fd))[-2])
#define FNTCHARCNT(fd) (((int *)(fd))[-3])
#define FONT_EXTRA_WORDS 3
To keep things simple, move all these macro definitions to ,
use four words instead of three, and initialize the fourth word in
newport_set_font() properly.
Many thanks to Greg Kroah-Hartman , who
reviewed and improved this series!
[1]: KASAN: global-out-of-bounds Read in fbcon_get_font
https://syzkaller.appspot.com/bug?id=08b8be45afea11888776f897895aef9ad1c3ecfd
Peilin Ye (3):
fbdev, newport_con: Move FONT_EXTRA_WORDS macros into linux/font.h
Fonts: Support FONT_EXTRA_WORDS macros for built-in fonts
fbcon: Fix global-out-of-bounds read in fbcon_get_font()
drivers/video/console/newport_con.c | 7 +--
drivers/video/fbdev/core/fbcon.c| 12
drivers/video/fbdev/core/fbcon.h| 7 ---
drivers/video/fbdev/core/fbcon_rotate.c | 1 +
drivers/video/fbdev/core/tileblit.c | 1 +
include/linux/font.h| 13 +
lib/fonts/font_10x18.c | 9 -
lib/fonts/font_6x10.c | 9 +
lib/fonts/font_6x11.c | 9 -
lib/fonts/font_7x14.c | 9 -
lib/fonts/font_8x16.c | 9 -
lib/fonts/font_8x8.c| 9 -
lib/fonts/font_acorn_8x8.c | 9 ++---
lib/fonts/font_mini_4x6.c | 8
lib/fonts/font_pearl_8x8.c | 9 -
lib/fonts/font_sun12x22.c | 9 -
lib/fonts/font_sun8x16.c| 7 ---
lib/fonts/font_ter16x32.c | 9 -
18 files changed, 79 insertions(+), 67 deletions(-)
--
2.25.1
