From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Jiri Slaby <jslaby@suse.com>, Dmitry Vyukov <dvyukov@google.com>,
linux-kernel@vger.kernel.org,
syzbot <syzbot+017265e8553724e514e8@syzkaller.appspotmail.com>
Subject: Re: [PATCH] vt: Reject zero-sized screen buffer size.
Date: Fri, 10 Jul 2020 13:36:58 +0200 [thread overview]
Message-ID: <20200710113658.GA1238355@kroah.com> (raw)
In-Reply-To: <7adf8aee-9bdd-8184-6cbe-291357677edd@i-love.sakura.ne.jp>
On Fri, Jul 10, 2020 at 08:31:42PM +0900, Tetsuo Handa wrote:
> On 2020/07/10 19:55, Greg Kroah-Hartman wrote:
> >> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> >> index 48a8199f7845..8497e9206607 100644
> >> --- a/drivers/tty/vt/vt.c
> >> +++ b/drivers/tty/vt/vt.c
> >> @@ -1126,7 +1126,7 @@ int vc_allocate(unsigned int currcons) /* return 0 on success */
> >> con_set_default_unimap(vc);
> >>
> >> vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL);
> >> - if (!vc->vc_screenbuf)
> >> + if (ZERO_OR_NULL_PTR(vc->vc_screenbuf))
> >
> > No, let's check this before we do kzalloc() please, that's just an odd
> > way of doing an allocation we shouldn't have had to do.
>
> OK. I can change to
>
> + if (vc->vc_screenbuf_size > KMALLOC_MAX_SIZE || !vc->vc_screenbuf_size)
> + goto err_free;
> vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_KERNEL);
> if (!vc->vc_screenbuf)
> goto err_free;
>
> like vc_do_resize() does. But I'm currently waiting for syzbot to test this patch, for
> I don't have an environment for reproducing this problem.
That looks much more sane, thanks.
>
> >
> >> goto err_free;
> >>
> >> /* If no drivers have overridden us and the user didn't pass a
> >> @@ -1212,7 +1212,7 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
> >> if (new_cols == vc->vc_cols && new_rows == vc->vc_rows)
> >> return 0;
> >>
> >> - if (new_screen_size > KMALLOC_MAX_SIZE)
> >> + if (new_screen_size > KMALLOC_MAX_SIZE || !new_screen_size)
> >> return -EINVAL;
> >> newscreen = kzalloc(new_screen_size, GFP_USER);
> >> if (!newscreen)
> >> @@ -3393,6 +3393,7 @@ static int __init con_init(void)
> >> INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> >> tty_port_init(&vc->port);
> >> visual_init(vc, currcons, 1);
> >> + /* Assuming vc->vc_screenbuf_size is sane here, for this is __init code. */
> >
> > Shouldn't we also check this here, or before we get here, too?
>
> This is an __init function. Can we somehow pass column=0 or row=0 ?
You could, it's much less likely, but why not catch this if you can?
> > Just checking the values and rejecting that as a valid screen size
> > should be sufficient.
>
> Hmm, where are we checking that column * row does not exceed UINT_MAX, given that
> "struct vc_data"->vc_{cols,rows,screenbuf_size} are "unsigned int" and we do
>
> vc->vc_size_row = vc->vc_cols << 1;
> vc->vc_screenbuf_size = vc->vc_rows * vc->vc_size_row;
>
> in visual_init() ? Don't we need to reject earlier?
Probably, it's some twisty code :(
next prev parent reply other threads:[~2020-07-10 11:37 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-10 5:53 [PATCH] vt: Reject zero-sized screen buffer size Tetsuo Handa
2020-07-10 5:56 ` fbconsole needs more parameter validations Tetsuo Handa
2020-07-10 10:56 ` Greg Kroah-Hartman
2020-07-11 6:16 ` Tetsuo Handa
2020-07-11 11:08 ` Tetsuo Handa
2020-07-12 11:10 ` [PATCH v3] vt: Reject zero-sized screen buffer size Tetsuo Handa
2020-07-12 11:10 ` [PATCH] fbdev: Detect integer underflow at "struct fbcon_ops"->clear_margins Tetsuo Handa
2020-07-14 7:22 ` Bartlomiej Zolnierkiewicz
2020-07-14 10:27 ` Tetsuo Handa
2020-07-14 13:37 ` Tetsuo Handa
2020-07-15 1:51 ` [PATCH v2] " Tetsuo Handa
2020-07-15 9:48 ` Dan Carpenter
2020-07-15 11:17 ` Tetsuo Handa
2020-07-15 14:02 ` Tetsuo Handa
2020-07-15 15:12 ` Dan Carpenter
2020-07-15 15:29 ` Tetsuo Handa
2020-07-16 10:00 ` Daniel Vetter
2020-07-16 11:27 ` Tetsuo Handa
2020-07-21 16:08 ` Greg Kroah-Hartman
2020-07-22 8:07 ` Daniel Vetter
2020-07-23 14:21 ` Greg Kroah-Hartman
2020-07-24 8:28 ` Bartlomiej Zolnierkiewicz
[not found] ` <c5bf6d5c-8d0a-8df5-2a11-38bf37a11d67@oracle.com>
2020-07-15 0:24 ` [PATCH] " Tetsuo Handa
2020-08-19 22:07 ` [PATCH v3] vt: Reject zero-sized screen buffer size Kees Cook
2020-07-10 10:55 ` [PATCH] " Greg Kroah-Hartman
2020-07-10 11:31 ` Tetsuo Handa
2020-07-10 11:36 ` Greg Kroah-Hartman [this message]
2020-07-10 14:34 ` [PATCH v2] " Tetsuo Handa
2020-07-20 15:40 ` Brooke Basile
2020-07-20 23:00 ` Tetsuo Handa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200710113658.GA1238355@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=dvyukov@google.com \
--cc=jslaby@suse.com \
--cc=linux-kernel@vger.kernel.org \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=syzbot+017265e8553724e514e8@syzkaller.appspotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox