[PATCH] tty: vt: add some NULL checks for vc_data

[PATCH] tty: vt: add some NULL checks for vc_data

[Hang Zhang]

vc_selection(), do_blank_screen() and scrollfront() all access "vc_data"
structures obtained from the global "vc_cons[fg_console].d", which can
be freed and nullified (e.g., in the error path of vc_allocate()). But
these functions don't have any NULL checks against the pointers before
dereferencing them, causing potentially use-after-free or null pointer
dereference.

Prevent these potential issues by placing NULL checks in these functions
before accessing "vc_data" structures. Similar checks can be found in
other functions like vt_console_print() and poke_blanked_console().

Signed-off-by: Hang Zhang <zh.nvgt@gmail.com>
---
 drivers/tty/vt/selection.c | 3 +++
 drivers/tty/vt/vt.c        | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
index 6ef22f01cc51..c727fd947683 100644
--- a/drivers/tty/vt/selection.c
+++ b/drivers/tty/vt/selection.c
@@ -319,6 +319,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
 {
 	int ps, pe;
 
+	if (!vc)
+		return 0;
+
 	poke_blanked_console();
 
 	if (v->sel_mode == TIOCL_SELCLEAR) {
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 981d2bfcf9a5..00f8fdc61e9f 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1493,6 +1493,8 @@ void scrollback(struct vc_data *vc)
 
 void scrollfront(struct vc_data *vc, int lines)
 {
+	if (!vc)
+		return;
 	if (!lines)
 		lines = vc->vc_rows / 2;
 	scrolldelta(lines);
@@ -4346,6 +4348,9 @@ void do_blank_screen(int entering_gfx)
 	struct vc_data *vc = vc_cons[fg_console].d;
 	int i;
 
+	if (!vc)
+		return;
+
 	might_sleep();
 
 	WARN_CONSOLE_UNLOCKED();
-- 
2.39.0

Re: [PATCH] tty: vt: add some NULL checks for vc_data

[Jiri Slaby]

On 29. 12. 22, 7:41, Hang Zhang wrote:
> vc_selection(), do_blank_screen() and scrollfront() all access "vc_data"
> structures obtained from the global "vc_cons[fg_console].d", which can
> be freed and nullified (e.g., in the error path of vc_allocate()). But
> these functions don't have any NULL checks against the pointers before
> dereferencing them, causing potentially use-after-free or null pointer
> dereference.

Could you elaborate under what circumstances is fg_console set to a 
non-allocated console?

> Prevent these potential issues by placing NULL checks in these functions
> before accessing "vc_data" structures. Similar checks can be found in
> other functions like vt_console_print() and poke_blanked_console().
> 
> Signed-off-by: Hang Zhang <zh.nvgt@gmail.com>
> ---
>   drivers/tty/vt/selection.c | 3 +++
>   drivers/tty/vt/vt.c        | 5 +++++
>   2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
> index 6ef22f01cc51..c727fd947683 100644
> --- a/drivers/tty/vt/selection.c
> +++ b/drivers/tty/vt/selection.c
> @@ -319,6 +319,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
>   {
>   	int ps, pe;
>   
> +	if (!vc)
> +		return 0;
> +
>   	poke_blanked_console();
>   
>   	if (v->sel_mode == TIOCL_SELCLEAR) {
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 981d2bfcf9a5..00f8fdc61e9f 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -1493,6 +1493,8 @@ void scrollback(struct vc_data *vc)
>   
>   void scrollfront(struct vc_data *vc, int lines)
>   {
> +	if (!vc)
> +		return;
>   	if (!lines)
>   		lines = vc->vc_rows / 2;
>   	scrolldelta(lines);
> @@ -4346,6 +4348,9 @@ void do_blank_screen(int entering_gfx)
>   	struct vc_data *vc = vc_cons[fg_console].d;
>   	int i;
>   
> +	if (!vc)
> +		return;
> +
>   	might_sleep();
>   
>   	WARN_CONSOLE_UNLOCKED();

thanks,
-- 
js
suse labs

Re: [PATCH] tty: vt: add some NULL checks for vc_data

[Hang Zhang]

On Tue, Jan 3, 2023 at 4:24 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 29. 12. 22, 7:41, Hang Zhang wrote:
> > vc_selection(), do_blank_screen() and scrollfront() all access "vc_data"
> > structures obtained from the global "vc_cons[fg_console].d", which can
> > be freed and nullified (e.g., in the error path of vc_allocate()). But
> > these functions don't have any NULL checks against the pointers before
> > dereferencing them, causing potentially use-after-free or null pointer
> > dereference.
>
> Could you elaborate under what circumstances is fg_console set to a
> non-allocated console?

Hi, Jiri, thank you for your reply! I am not a developer for tty
subsystem, so the reasoning here is based on my best-effort code
reading. Please correct me if I am wrong.

This patch is based on several observations:

(1) at the beginning of vc_selection() (where one NULL check is
inserted in this patch), poke_blanked_console() is invoked, which
explicitly checks whether "vc_cons[fg_console].d" is NULL, suggesting
the possibility of "fg_console" associated with an unallocated console
at this point. However, poke_blanked_console() returns "void", so
even if "fg_console" is NULL, after returning to vc_selection(),
it will just keep executing, resulting in the possible NULL pointer
dereference later ("vc" in vc_selection() can be "vc_cons[fg_console].d"
if called from set_selection_kernel()). So this patch actually tries
to make the already existing NULL check take effect on the control
flow (e.g., early return if NULL).

(2) a similar NULL check for "vc_cons[fg_console].d" can also be found
in do_unblank_screen() ("if (!vc_cons_allocated(fg_console))") before
accessing the corresponding "vc_data". I do notice that the NULL check
has a comment "/* impossible */", but the check has not been removed so
far. My guess is that there might still be a chance that it can be
unallocated at that point.

(3) regarding how "fg_console" can be unallocated, one place I noticed
is vc_allocate() (invoked from vt_ioctl$VT_ACTIVATE), where the user
can specify the index of "vc_cons" to be activated (can be "fg_console"
in theory). If any error happens and the code under the label "err_free"
is executed, then "vc_cons[fg_console].d" can be freed and nullified.
But I am not sure about the exact scenario/flow to make "fg_console"
unallocated, as this seems to need a thorough understanding of all places
manipulating "fg_console" across the whole subsystem (including the
synchronization between them).

So, in conclusion, I cannot exclude the possibility of potential
NULL dereference/use-after-free issues with my limited domain
knowledge. Inspired by existing checks in the code as aforementioned,
I think it's safer to add some additional NULL checks. If the developers
finally decide that "fg_console" cannot be unallocated in reality,
then maybe we should also consider removing some unnecessary checks.

>
> > Prevent these potential issues by placing NULL checks in these functions
> > before accessing "vc_data" structures. Similar checks can be found in
> > other functions like vt_console_print() and poke_blanked_console().
> >
> > Signed-off-by: Hang Zhang <zh.nvgt@gmail.com>
> > ---
> >   drivers/tty/vt/selection.c | 3 +++
> >   drivers/tty/vt/vt.c        | 5 +++++
> >   2 files changed, 8 insertions(+)
> >
> > diff --git a/drivers/tty/vt/selection.c b/drivers/tty/vt/selection.c
> > index 6ef22f01cc51..c727fd947683 100644
> > --- a/drivers/tty/vt/selection.c
> > +++ b/drivers/tty/vt/selection.c
> > @@ -319,6 +319,9 @@ static int vc_selection(struct vc_data *vc, struct tiocl_selection *v,
> >   {
> >       int ps, pe;
> >
> > +     if (!vc)
> > +             return 0;
> > +
> >       poke_blanked_console();
> >
> >       if (v->sel_mode == TIOCL_SELCLEAR) {
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 981d2bfcf9a5..00f8fdc61e9f 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -1493,6 +1493,8 @@ void scrollback(struct vc_data *vc)
> >
> >   void scrollfront(struct vc_data *vc, int lines)
> >   {
> > +     if (!vc)
> > +             return;
> >       if (!lines)
> >               lines = vc->vc_rows / 2;
> >       scrolldelta(lines);
> > @@ -4346,6 +4348,9 @@ void do_blank_screen(int entering_gfx)
> >       struct vc_data *vc = vc_cons[fg_console].d;
> >       int i;
> >
> > +     if (!vc)
> > +             return;
> > +
> >       might_sleep();
> >
> >       WARN_CONSOLE_UNLOCKED();
>
> thanks,
> --
> js
> suse labs
>

Re: [PATCH] tty: vt: add some NULL checks for vc_data

[Greg Kroah-Hartman]

On Tue, Jan 03, 2023 at 10:01:15PM -0500, Hang Zhang wrote:
> On Tue, Jan 3, 2023 at 4:24 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> >
> > On 29. 12. 22, 7:41, Hang Zhang wrote:
> > > vc_selection(), do_blank_screen() and scrollfront() all access "vc_data"
> > > structures obtained from the global "vc_cons[fg_console].d", which can
> > > be freed and nullified (e.g., in the error path of vc_allocate()). But
> > > these functions don't have any NULL checks against the pointers before
> > > dereferencing them, causing potentially use-after-free or null pointer
> > > dereference.
> >
> > Could you elaborate under what circumstances is fg_console set to a
> > non-allocated console?
> 
> Hi, Jiri, thank you for your reply! I am not a developer for tty
> subsystem, so the reasoning here is based on my best-effort code
> reading. Please correct me if I am wrong.
> 
> This patch is based on several observations:
> 
> (1) at the beginning of vc_selection() (where one NULL check is
> inserted in this patch), poke_blanked_console() is invoked, which
> explicitly checks whether "vc_cons[fg_console].d" is NULL, suggesting
> the possibility of "fg_console" associated with an unallocated console
> at this point. However, poke_blanked_console() returns "void", so
> even if "fg_console" is NULL, after returning to vc_selection(),
> it will just keep executing, resulting in the possible NULL pointer
> dereference later ("vc" in vc_selection() can be "vc_cons[fg_console].d"
> if called from set_selection_kernel()). So this patch actually tries
> to make the already existing NULL check take effect on the control
> flow (e.g., early return if NULL).

But again, how can that value ever be NULL?

And why are you returning "success" if it is?

> (2) a similar NULL check for "vc_cons[fg_console].d" can also be found
> in do_unblank_screen() ("if (!vc_cons_allocated(fg_console))") before
> accessing the corresponding "vc_data". I do notice that the NULL check
> has a comment "/* impossible */", but the check has not been removed so
> far. My guess is that there might still be a chance that it can be
> unallocated at that point.

Please verify that this really ever could be NULL or not.

thanks,

greg k-h

Re: [PATCH] tty: vt: add some NULL checks for vc_data

[Hang Zhang]

On Fri, Jan 6, 2023 at 6:30 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jan 03, 2023 at 10:01:15PM -0500, Hang Zhang wrote:
> > On Tue, Jan 3, 2023 at 4:24 AM Jiri Slaby <jirislaby@kernel.org> wrote:
> > >
> > > On 29. 12. 22, 7:41, Hang Zhang wrote:
> > > > vc_selection(), do_blank_screen() and scrollfront() all access "vc_data"
> > > > structures obtained from the global "vc_cons[fg_console].d", which can
> > > > be freed and nullified (e.g., in the error path of vc_allocate()). But
> > > > these functions don't have any NULL checks against the pointers before
> > > > dereferencing them, causing potentially use-after-free or null pointer
> > > > dereference.
> > >
> > > Could you elaborate under what circumstances is fg_console set to a
> > > non-allocated console?
> >
> > Hi, Jiri, thank you for your reply! I am not a developer for tty
> > subsystem, so the reasoning here is based on my best-effort code
> > reading. Please correct me if I am wrong.
> >
> > This patch is based on several observations:
> >
> > (1) at the beginning of vc_selection() (where one NULL check is
> > inserted in this patch), poke_blanked_console() is invoked, which
> > explicitly checks whether "vc_cons[fg_console].d" is NULL, suggesting
> > the possibility of "fg_console" associated with an unallocated console
> > at this point. However, poke_blanked_console() returns "void", so
> > even if "fg_console" is NULL, after returning to vc_selection(),
> > it will just keep executing, resulting in the possible NULL pointer
> > dereference later ("vc" in vc_selection() can be "vc_cons[fg_console].d"
> > if called from set_selection_kernel()). So this patch actually tries
> > to make the already existing NULL check take effect on the control
> > flow (e.g., early return if NULL).
>
> But again, how can that value ever be NULL?
>
> And why are you returning "success" if it is?

Hi, Greg. Thank you for your reply. When writing this patch, we didn't have
many special considerations for the return value - the main purpose is to make
it return early to prevent the later potential null pointer
dereference. The return
value is borrowed from other early return branches within
vc_selection() that all
return 0. We can certainly change it to a specific error code according to
developers' comments.

Sure, we will try to put more effort into investigating the
possibility of the NULL
pointer. Due to our limited domain knowledge, any help/insight from the
developers and maintainers is highly appreciated. Thank you very much!

Thanks,
Hang