NACK/Cmnt: [PATCH] tty: make FONTX ioctl use the tty pointer they were actually passed
Stefan Bader
stefan.bader at canonical.com
Tue Feb 23 07:34:48 UTC 2021
On 22.02.21 22:01, Thadeu Lima de Souza Cascardo wrote:
> On Mon, Feb 22, 2021 at 01:26:17PM -0700, Tim Gardner wrote:
>> From: Linus Torvalds <torvalds at linux-foundation.org>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1908150
>>
>> commit 90bfdeef83f1d6c696039b6a917190dcbbad3220 upstream.
>>
>> Some of the font tty ioctl's always used the current foreground VC for
>> their operations. Don't do that then.
>>
>> This fixes a data race on fg_console.
>>
>> Side note: both Michael Ellerman and Jiri Slaby point out that all these
>> ioctls are deprecated, and should probably have been removed long ago,
>> and everything seems to be using the KDFONTOP ioctl instead.
>>
>> In fact, Michael points out that it looks like busybox's loadfont
>> program seems to have switched over to using KDFONTOP exactly _because_
>> of this bug (ahem.. 12 years ago ;-).
>>
>> Reported-by: Minh Yuan <yuanmingbuaa at gmail.com>
>> Acked-by: Michael Ellerman <mpe at ellerman.id.au>
>> Acked-by: Jiri Slaby <jirislaby at kernel.org>
>> Cc: Greg KH <greg at kroah.com>
>> Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
>> Signed-off-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>> Signed-off-by: Kamal Mostafa <kamal at canonical.com>
>> Signed-off-by: Ian May <ian.may at canonical.com>
>> (cherry picked from groovy/linux commit 161c231cb45fb23c5fbfcdd9d29cb72b6047ff61)
>
> This should probably be a backport of upstream commit
> 90bfdeef83f1d6c696039b6a917190dcbbad3220, instead. I see how it's worth taking
> up fixes that were already ported to a closer series when conflicts are not the
> most trivial. They have been tested and such. And pointing it out is worth it
> too.
>
> Now, we don't have a standard when doing that. Unless we are picking it up from
> upstream stable series, where we do:
>
> (cherry picked from commit 161c231cb45fb23c5fbfcdd9d29cb72b6047ff61 linux-5.8.y)
>
> The thing I really care about on the message format is that the CVE autotriager
> will do its work, and find out we applied the upstream fix. That is guaranteed
> by the line, which will be matched by its code:
>
> commit 90bfdeef83f1d6c696039b6a917190dcbbad3220 upstream.
Not sure this is the complete truth. At least in the past above form was only
used by upstream stable and anything we did used the cherry pick/backport line
and for that it had to be the correct format.
-Stefan
>
> Acked-by: Thadeu Lima de Souza Cascardo <cascardo at canonical.com>
>
>> CVE-2020-25668
>> Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
>> ---
>> drivers/tty/vt/vt_ioctl.c | 32 +++++++++++++++++---------------
>> 1 file changed, 17 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
>> index e9ebe1520a56..142f3a5a01fc 100644
>> --- a/drivers/tty/vt/vt_ioctl.c
>> +++ b/drivers/tty/vt/vt_ioctl.c
>> @@ -244,7 +244,7 @@ int vt_waitactive(int n)
>>
>>
>> static inline int
>> -do_fontx_ioctl(int cmd, struct consolefontdesc __user *user_cfd, int perm, struct console_font_op *op)
>> +do_fontx_ioctl(struct vc_data *vc, int cmd, struct consolefontdesc __user *user_cfd, int perm, struct console_font_op *op)
>> {
>> struct consolefontdesc cfdarg;
>> int i;
>> @@ -262,15 +262,16 @@ do_fontx_ioctl(int cmd, struct consolefontdesc __user *user_cfd, int perm, struc
>> op->height = cfdarg.charheight;
>> op->charcount = cfdarg.charcount;
>> op->data = cfdarg.chardata;
>> - return con_font_op(vc_cons[fg_console].d, op);
>> - case GIO_FONTX: {
>> + return con_font_op(vc, op);
>> +
>> + case GIO_FONTX:
>> op->op = KD_FONT_OP_GET;
>> op->flags = KD_FONT_FLAG_OLD;
>> op->width = 8;
>> op->height = cfdarg.charheight;
>> op->charcount = cfdarg.charcount;
>> op->data = cfdarg.chardata;
>> - i = con_font_op(vc_cons[fg_console].d, op);
>> + i = con_font_op(vc, op);
>> if (i)
>> return i;
>> cfdarg.charheight = op->height;
>> @@ -278,7 +279,6 @@ do_fontx_ioctl(int cmd, struct consolefontdesc __user *user_cfd, int perm, struc
>> if (copy_to_user(user_cfd, &cfdarg, sizeof(struct consolefontdesc)))
>> return -EFAULT;
>> return 0;
>> - }
>> }
>> return -EINVAL;
>> }
>> @@ -914,7 +914,7 @@ int vt_ioctl(struct tty_struct *tty,
>> op.height = 0;
>> op.charcount = 256;
>> op.data = up;
>> - ret = con_font_op(vc_cons[fg_console].d, &op);
>> + ret = con_font_op(vc, &op);
>> break;
>> }
>>
>> @@ -925,7 +925,7 @@ int vt_ioctl(struct tty_struct *tty,
>> op.height = 32;
>> op.charcount = 256;
>> op.data = up;
>> - ret = con_font_op(vc_cons[fg_console].d, &op);
>> + ret = con_font_op(vc, &op);
>> break;
>> }
>>
>> @@ -942,7 +942,7 @@ int vt_ioctl(struct tty_struct *tty,
>>
>> case PIO_FONTX:
>> case GIO_FONTX:
>> - ret = do_fontx_ioctl(cmd, up, perm, &op);
>> + ret = do_fontx_ioctl(vc, cmd, up, perm, &op);
>> break;
>>
>> case PIO_FONTRESET:
>> @@ -959,11 +959,11 @@ int vt_ioctl(struct tty_struct *tty,
>> {
>> op.op = KD_FONT_OP_SET_DEFAULT;
>> op.data = NULL;
>> - ret = con_font_op(vc_cons[fg_console].d, &op);
>> + ret = con_font_op(vc, &op);
>> if (ret)
>> break;
>> console_lock();
>> - con_set_default_unimap(vc_cons[fg_console].d);
>> + con_set_default_unimap(vc);
>> console_unlock();
>> break;
>> }
>> @@ -1090,8 +1090,9 @@ struct compat_consolefontdesc {
>> };
>>
>> static inline int
>> -compat_fontx_ioctl(int cmd, struct compat_consolefontdesc __user *user_cfd,
>> - int perm, struct console_font_op *op)
>> +compat_fontx_ioctl(struct vc_data *vc, int cmd,
>> + struct compat_consolefontdesc __user *user_cfd,
>> + int perm, struct console_font_op *op)
>> {
>> struct compat_consolefontdesc cfdarg;
>> int i;
>> @@ -1109,7 +1110,8 @@ compat_fontx_ioctl(int cmd, struct compat_consolefontdesc __user *user_cfd,
>> op->height = cfdarg.charheight;
>> op->charcount = cfdarg.charcount;
>> op->data = compat_ptr(cfdarg.chardata);
>> - return con_font_op(vc_cons[fg_console].d, op);
>> + return con_font_op(vc, op);
>> +
>> case GIO_FONTX:
>> op->op = KD_FONT_OP_GET;
>> op->flags = KD_FONT_FLAG_OLD;
>> @@ -1117,7 +1119,7 @@ compat_fontx_ioctl(int cmd, struct compat_consolefontdesc __user *user_cfd,
>> op->height = cfdarg.charheight;
>> op->charcount = cfdarg.charcount;
>> op->data = compat_ptr(cfdarg.chardata);
>> - i = con_font_op(vc_cons[fg_console].d, op);
>> + i = con_font_op(vc, op);
>> if (i)
>> return i;
>> cfdarg.charheight = op->height;
>> @@ -1207,7 +1209,7 @@ long vt_compat_ioctl(struct tty_struct *tty,
>> */
>> case PIO_FONTX:
>> case GIO_FONTX:
>> - return compat_fontx_ioctl(cmd, up, perm, &op);
>> + return compat_fontx_ioctl(vc, cmd, up, perm, &op);
>>
>> case KDFONTOP:
>> return compat_kdfontop_ioctl(up, perm, &op, vc);
>> --
>> 2.17.1
>>
>>
>> --
>> kernel-team mailing list
>> kernel-team at lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210223/4e60ac9c/attachment-0001.sig>
More information about the kernel-team
mailing list