Re: [PATCH] vt: Fix a missing-check bug in drivers/tty/vt/vt.c file of Linux 5.0.14

[Greg KH]

On Sat, May 11, 2019 at 09:21:39AM +0800, Gen Zhang wrote:
> On Fri, May 10, 2019 at 11:12:50PM +0800, Greg KH <
> gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >Still impossible to apply :(
> >
> >Also, what about Dave's response to you?  This really can never be hit,
> >like other early-init tty allocations that we do not check because of
> >this issue, correct?
> >
> >thanks,
> >
> >greg k-h
> 1. Cannot imply the patch
> I pulled the latest kernel from github(commit
> 1fb3b526df3bd7647e7854915ae6b22299408baf), and patched with
> **************************************
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> 
> @@ -3322,10 +3322,14 @@ static int __init con_init(void)
> 
>   for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
>   vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> + if (!vc_cons[currcons].d || !vc)
> + goto err_vc;
>   INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
>   tty_port_init(&vc->port);
>   visual_init(vc, currcons, 1);
>   vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> + if (!vc->vc_screenbuf)
> + goto err_vc_screenbuf;
>   vc_init(vc, vc->vc_rows, vc->vc_cols,
>   currcons || !vc->vc_sw->con_save_screen);
>   }
> @@ -3347,6 +3351,14 @@ static int __init con_init(void)
>   register_console(&vt_console_driver);
>  #endif
>   return 0;
> +err_vc:
> + console_unlock();
> + return -ENOMEM;
> +err_vc_screenbuf:
> + console_unlock();
> + kfree(vc);
> + vc_cons[currcons].d = NULL;
> + return -ENOMEM;
>  }
>  console_initcall(con_init);
> 
> 
> **************************************
> (It is possible that you missed the last line?)

Look at the patch above, all of the whitespace is damaged.  There is no
way you took the raw email and then were able to apply that to the
kernel tree.

You can not cut/paste patches into gmail, please read the kernel
Documentation file all about email clients and how to get them to work
properly to send patches.

> 2. David's response
> In my humble opinion, whatever the cause is, theoratically, there is a
> possibility that memory allocation (e.g. kzalloc()) can be failed.
> I don't think it is related to whether we are in the early-initial stage or
> not.

But it is directly related.

> Once the allocated pointer (e.g. vc) is deferenced, the kernel might go
> wrong.
> And in this case, variable vc_cons[currcons].d, vc and vc->vc_screenbuf is
> deferenced after allocation.
> Thus I think we should add the allocation check to prevent null pointer
> deference.

For most problems, yes, if you can successfully unwind and continue on
with a working system.  Will that happen here?

thanks,

greg k-h