| Message ID | 20201203212346.2726267-2-cascardo@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Groovy,Focal/linux-oem-5.6] vt: Disable KD_FONT_OP_COPY | expand |
On 03.12.20 22:23, Thadeu Lima de Souza Cascardo wrote: > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > It's buggy: > > On Fri, Nov 06, 2020 at 10:30:08PM +0800, Minh Yuan wrote: >> We recently discovered a slab-out-of-bounds read in fbcon in the latest >> kernel ( v5.10-rc2 for now ). The root cause of this vulnerability is that >> "fbcon_do_set_font" did not handle "vc->vc_font.data" and >> "vc->vc_font.height" correctly, and the patch >> <https://lkml.org/lkml/2020/9/27/223> for VT_RESIZEX can't handle this >> issue. >> >> Specifically, we use KD_FONT_OP_SET to set a small font.data for tty6, and >> use KD_FONT_OP_SET again to set a large font.height for tty1. After that, >> we use KD_FONT_OP_COPY to assign tty6's vc_font.data to tty1's vc_font.data >> in "fbcon_do_set_font", while tty1 retains the original larger >> height. Obviously, this will cause an out-of-bounds read, because we can >> access a smaller vc_font.data with a larger vc_font.height. > > Further there was only one user ever. > - Android's loadfont, busybox and console-tools only ever use OP_GET > and OP_SET > - fbset documentation only mentions the kernel cmdline font: option, > not anything else. > - systemd used OP_COPY before release 232 published in Nov 2016 > > Now unfortunately the crucial report seems to have gone down with > gmane, and the commit message doesn't say much. But the pull request > hints at OP_COPY being broken > > https://github.com/systemd/systemd/pull/3651 > > So in other words, this never worked, and the only project which > foolishly every tried to use it, realized that rather quickly too. > > Instead of trying to fix security issues here on dead code by adding > missing checks, fix the entire thing by removing the functionality. > > Note that systemd code using the OP_COPY function ignored the return > value, so it doesn't matter what we're doing here really - just in > case a lone server somewhere happens to be extremely unlucky and > running an affected old version of systemd. The relevant code from > font_copy_to_all_vcs() in systemd was: > > /* copy font from active VT, where the font was uploaded to */ > cfo.op = KD_FONT_OP_COPY; > cfo.height = vcs.v_active-1; /* tty1 == index 0 */ > (void) ioctl(vcfd, KDFONTOP, &cfo); > > Note this just disables the ioctl, garbage collecting the now unused > callbacks is left for -next. > > v2: Tetsuo found the old mail, which allowed me to find it on another > archive. Add the link too. > > Acked-by: Peilin Ye <yepeilin.cs@gmail.com> > Reported-by: Minh Yuan <yuanmingbuaa@gmail.com> > References: https://lists.freedesktop.org/archives/systemd-devel/2016-June/036935.html > References: https://github.com/systemd/systemd/pull/3651 > Cc: Greg KH <greg@kroah.com> > Cc: Peilin Ye <yepeilin.cs@gmail.com> > Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Link: https://lore.kernel.org/r/20201108153806.3140315-1-daniel.vetter@ffwll.ch > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > (cherry picked from commit 3c4e0dff2095c579b142d5a0693257f1c58b4804) > CVE-2020-28974 > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > drivers/tty/vt/vt.c | 24 ++---------------------- > 1 file changed, 2 insertions(+), 22 deletions(-) > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c > index bb0623d60cc0..74e3945e7146 100644 > --- a/drivers/tty/vt/vt.c > +++ b/drivers/tty/vt/vt.c > @@ -4643,27 +4643,6 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op) > return rc; > } > > -static int con_font_copy(struct vc_data *vc, struct console_font_op *op) > -{ > - int con = op->height; > - int rc; > - > - > - console_lock(); > - if (vc->vc_mode != KD_TEXT) > - rc = -EINVAL; > - else if (!vc->vc_sw->con_font_copy) > - rc = -ENOSYS; > - else if (con < 0 || !vc_cons_allocated(con)) > - rc = -ENOTTY; > - else if (con == vc->vc_num) /* nothing to do */ > - rc = 0; > - else > - rc = vc->vc_sw->con_font_copy(vc, con); > - console_unlock(); > - return rc; > -} > - > int con_font_op(struct vc_data *vc, struct console_font_op *op) > { > switch (op->op) { > @@ -4674,7 +4653,8 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op) > case KD_FONT_OP_SET_DEFAULT: > return con_font_default(vc, op); > case KD_FONT_OP_COPY: > - return con_font_copy(vc, op); > + /* was buggy and never really used */ > + return -EINVAL; > } > return -ENOSYS; > } >
On 03.12.20 22:23, Thadeu Lima de Souza Cascardo wrote: > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > It's buggy: > > On Fri, Nov 06, 2020 at 10:30:08PM +0800, Minh Yuan wrote: >> We recently discovered a slab-out-of-bounds read in fbcon in the latest >> kernel ( v5.10-rc2 for now ). The root cause of this vulnerability is that >> "fbcon_do_set_font" did not handle "vc->vc_font.data" and >> "vc->vc_font.height" correctly, and the patch >> <https://lkml.org/lkml/2020/9/27/223> for VT_RESIZEX can't handle this >> issue. >> >> Specifically, we use KD_FONT_OP_SET to set a small font.data for tty6, and >> use KD_FONT_OP_SET again to set a large font.height for tty1. After that, >> we use KD_FONT_OP_COPY to assign tty6's vc_font.data to tty1's vc_font.data >> in "fbcon_do_set_font", while tty1 retains the original larger >> height. Obviously, this will cause an out-of-bounds read, because we can >> access a smaller vc_font.data with a larger vc_font.height. > > Further there was only one user ever. > - Android's loadfont, busybox and console-tools only ever use OP_GET > and OP_SET > - fbset documentation only mentions the kernel cmdline font: option, > not anything else. > - systemd used OP_COPY before release 232 published in Nov 2016 > > Now unfortunately the crucial report seems to have gone down with > gmane, and the commit message doesn't say much. But the pull request > hints at OP_COPY being broken > > https://github.com/systemd/systemd/pull/3651 > > So in other words, this never worked, and the only project which > foolishly every tried to use it, realized that rather quickly too. > > Instead of trying to fix security issues here on dead code by adding > missing checks, fix the entire thing by removing the functionality. > > Note that systemd code using the OP_COPY function ignored the return > value, so it doesn't matter what we're doing here really - just in > case a lone server somewhere happens to be extremely unlucky and > running an affected old version of systemd. The relevant code from > font_copy_to_all_vcs() in systemd was: > > /* copy font from active VT, where the font was uploaded to */ > cfo.op = KD_FONT_OP_COPY; > cfo.height = vcs.v_active-1; /* tty1 == index 0 */ > (void) ioctl(vcfd, KDFONTOP, &cfo); > > Note this just disables the ioctl, garbage collecting the now unused > callbacks is left for -next. > > v2: Tetsuo found the old mail, which allowed me to find it on another > archive. Add the link too. > > Acked-by: Peilin Ye <yepeilin.cs@gmail.com> > Reported-by: Minh Yuan <yuanmingbuaa@gmail.com> > References: https://lists.freedesktop.org/archives/systemd-devel/2016-June/036935.html > References: https://github.com/systemd/systemd/pull/3651 > Cc: Greg KH <greg@kroah.com> > Cc: Peilin Ye <yepeilin.cs@gmail.com> > Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Link: https://lore.kernel.org/r/20201108153806.3140315-1-daniel.vetter@ffwll.ch > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > (cherry picked from commit 3c4e0dff2095c579b142d5a0693257f1c58b4804) > CVE-2020-28974 > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > drivers/tty/vt/vt.c | 24 ++---------------------- > 1 file changed, 2 insertions(+), 22 deletions(-) > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c > index bb0623d60cc0..74e3945e7146 100644 > --- a/drivers/tty/vt/vt.c > +++ b/drivers/tty/vt/vt.c > @@ -4643,27 +4643,6 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op) > return rc; > } > > -static int con_font_copy(struct vc_data *vc, struct console_font_op *op) > -{ > - int con = op->height; > - int rc; > - > - > - console_lock(); > - if (vc->vc_mode != KD_TEXT) > - rc = -EINVAL; > - else if (!vc->vc_sw->con_font_copy) > - rc = -ENOSYS; > - else if (con < 0 || !vc_cons_allocated(con)) > - rc = -ENOTTY; > - else if (con == vc->vc_num) /* nothing to do */ > - rc = 0; > - else > - rc = vc->vc_sw->con_font_copy(vc, con); > - console_unlock(); > - return rc; > -} > - > int con_font_op(struct vc_data *vc, struct console_font_op *op) > { > switch (op->op) { > @@ -4674,7 +4653,8 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op) > case KD_FONT_OP_SET_DEFAULT: > return con_font_default(vc, op); > case KD_FONT_OP_COPY: > - return con_font_copy(vc, op); > + /* was buggy and never really used */ > + return -EINVAL; > } > return -ENOSYS; > } >
Applied to groovy/linux Thanks, Ian On 2020-12-03 18:23:46 , Thadeu Lima de Souza Cascardo wrote: > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > It's buggy: > > On Fri, Nov 06, 2020 at 10:30:08PM +0800, Minh Yuan wrote: > > We recently discovered a slab-out-of-bounds read in fbcon in the latest > > kernel ( v5.10-rc2 for now ). The root cause of this vulnerability is that > > "fbcon_do_set_font" did not handle "vc->vc_font.data" and > > "vc->vc_font.height" correctly, and the patch > > <https://lkml.org/lkml/2020/9/27/223> for VT_RESIZEX can't handle this > > issue. > > > > Specifically, we use KD_FONT_OP_SET to set a small font.data for tty6, and > > use KD_FONT_OP_SET again to set a large font.height for tty1. After that, > > we use KD_FONT_OP_COPY to assign tty6's vc_font.data to tty1's vc_font.data > > in "fbcon_do_set_font", while tty1 retains the original larger > > height. Obviously, this will cause an out-of-bounds read, because we can > > access a smaller vc_font.data with a larger vc_font.height. > > Further there was only one user ever. > - Android's loadfont, busybox and console-tools only ever use OP_GET > and OP_SET > - fbset documentation only mentions the kernel cmdline font: option, > not anything else. > - systemd used OP_COPY before release 232 published in Nov 2016 > > Now unfortunately the crucial report seems to have gone down with > gmane, and the commit message doesn't say much. But the pull request > hints at OP_COPY being broken > > https://github.com/systemd/systemd/pull/3651 > > So in other words, this never worked, and the only project which > foolishly every tried to use it, realized that rather quickly too. > > Instead of trying to fix security issues here on dead code by adding > missing checks, fix the entire thing by removing the functionality. > > Note that systemd code using the OP_COPY function ignored the return > value, so it doesn't matter what we're doing here really - just in > case a lone server somewhere happens to be extremely unlucky and > running an affected old version of systemd. The relevant code from > font_copy_to_all_vcs() in systemd was: > > /* copy font from active VT, where the font was uploaded to */ > cfo.op = KD_FONT_OP_COPY; > cfo.height = vcs.v_active-1; /* tty1 == index 0 */ > (void) ioctl(vcfd, KDFONTOP, &cfo); > > Note this just disables the ioctl, garbage collecting the now unused > callbacks is left for -next. > > v2: Tetsuo found the old mail, which allowed me to find it on another > archive. Add the link too. > > Acked-by: Peilin Ye <yepeilin.cs@gmail.com> > Reported-by: Minh Yuan <yuanmingbuaa@gmail.com> > References: https://lists.freedesktop.org/archives/systemd-devel/2016-June/036935.html > References: https://github.com/systemd/systemd/pull/3651 > Cc: Greg KH <greg@kroah.com> > Cc: Peilin Ye <yepeilin.cs@gmail.com> > Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Link: https://lore.kernel.org/r/20201108153806.3140315-1-daniel.vetter@ffwll.ch > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > (cherry picked from commit 3c4e0dff2095c579b142d5a0693257f1c58b4804) > CVE-2020-28974 > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > --- > drivers/tty/vt/vt.c | 24 ++---------------------- > 1 file changed, 2 insertions(+), 22 deletions(-) > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c > index bb0623d60cc0..74e3945e7146 100644 > --- a/drivers/tty/vt/vt.c > +++ b/drivers/tty/vt/vt.c > @@ -4643,27 +4643,6 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op) > return rc; > } > > -static int con_font_copy(struct vc_data *vc, struct console_font_op *op) > -{ > - int con = op->height; > - int rc; > - > - > - console_lock(); > - if (vc->vc_mode != KD_TEXT) > - rc = -EINVAL; > - else if (!vc->vc_sw->con_font_copy) > - rc = -ENOSYS; > - else if (con < 0 || !vc_cons_allocated(con)) > - rc = -ENOTTY; > - else if (con == vc->vc_num) /* nothing to do */ > - rc = 0; > - else > - rc = vc->vc_sw->con_font_copy(vc, con); > - console_unlock(); > - return rc; > -} > - > int con_font_op(struct vc_data *vc, struct console_font_op *op) > { > switch (op->op) { > @@ -4674,7 +4653,8 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op) > case KD_FONT_OP_SET_DEFAULT: > return con_font_default(vc, op); > case KD_FONT_OP_COPY: > - return con_font_copy(vc, op); > + /* was buggy and never really used */ > + return -EINVAL; > } > return -ENOSYS; > } > -- > 2.27.0 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c index bb0623d60cc0..74e3945e7146 100644 --- a/drivers/tty/vt/vt.c +++ b/drivers/tty/vt/vt.c @@ -4643,27 +4643,6 @@ static int con_font_default(struct vc_data *vc, struct console_font_op *op) return rc; } -static int con_font_copy(struct vc_data *vc, struct console_font_op *op) -{ - int con = op->height; - int rc; - - - console_lock(); - if (vc->vc_mode != KD_TEXT) - rc = -EINVAL; - else if (!vc->vc_sw->con_font_copy) - rc = -ENOSYS; - else if (con < 0 || !vc_cons_allocated(con)) - rc = -ENOTTY; - else if (con == vc->vc_num) /* nothing to do */ - rc = 0; - else - rc = vc->vc_sw->con_font_copy(vc, con); - console_unlock(); - return rc; -} - int con_font_op(struct vc_data *vc, struct console_font_op *op) { switch (op->op) { @@ -4674,7 +4653,8 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op) case KD_FONT_OP_SET_DEFAULT: return con_font_default(vc, op); case KD_FONT_OP_COPY: - return con_font_copy(vc, op); + /* was buggy and never really used */ + return -EINVAL; } return -ENOSYS; }