* [PATCH] tty: vt: add some NULL checks for vc_data @ 2022-12-29 6:41 Hang Zhang 2023-01-03 9:24 ` Jiri Slaby 0 siblings, 1 reply; 5+ messages in thread From: Hang Zhang @ 2022-12-29 6:41 UTC (permalink / raw) Cc: Greg Kroah-Hartman, Jiri Slaby, Ilpo Järvinen, Hang Zhang, Daniel Vetter, Yangxi Xiang, Xuezhi Zhang, Helge Deller, Tetsuo Handa, linux-kernel 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tty: vt: add some NULL checks for vc_data 2022-12-29 6:41 [PATCH] tty: vt: add some NULL checks for vc_data Hang Zhang @ 2023-01-03 9:24 ` Jiri Slaby 2023-01-04 3:01 ` Hang Zhang 0 siblings, 1 reply; 5+ messages in thread From: Jiri Slaby @ 2023-01-03 9:24 UTC (permalink / raw) To: Hang Zhang Cc: Greg Kroah-Hartman, Ilpo Järvinen, Daniel Vetter, Yangxi Xiang, Xuezhi Zhang, Helge Deller, Tetsuo Handa, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tty: vt: add some NULL checks for vc_data 2023-01-03 9:24 ` Jiri Slaby @ 2023-01-04 3:01 ` Hang Zhang 2023-01-06 11:30 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: Hang Zhang @ 2023-01-04 3:01 UTC (permalink / raw) To: Jiri Slaby Cc: Greg Kroah-Hartman, Ilpo Järvinen, Daniel Vetter, Yangxi Xiang, Xuezhi Zhang, Helge Deller, Tetsuo Handa, linux-kernel 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tty: vt: add some NULL checks for vc_data 2023-01-04 3:01 ` Hang Zhang @ 2023-01-06 11:30 ` Greg Kroah-Hartman 2023-01-06 17:39 ` Hang Zhang 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2023-01-06 11:30 UTC (permalink / raw) To: Hang Zhang Cc: Jiri Slaby, Ilpo Järvinen, Daniel Vetter, Yangxi Xiang, Xuezhi Zhang, Helge Deller, Tetsuo Handa, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tty: vt: add some NULL checks for vc_data 2023-01-06 11:30 ` Greg Kroah-Hartman @ 2023-01-06 17:39 ` Hang Zhang 0 siblings, 0 replies; 5+ messages in thread From: Hang Zhang @ 2023-01-06 17:39 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Jiri Slaby, Ilpo Järvinen, Daniel Vetter, Yangxi Xiang, Xuezhi Zhang, Helge Deller, Tetsuo Handa, linux-kernel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-01-06 17:39 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-29 6:41 [PATCH] tty: vt: add some NULL checks for vc_data Hang Zhang 2023-01-03 9:24 ` Jiri Slaby 2023-01-04 3:01 ` Hang Zhang 2023-01-06 11:30 ` Greg Kroah-Hartman 2023-01-06 17:39 ` Hang Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).