Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221229064153.23511-1-zh.nvgt@gmail.com> <182d36d5-df77-2479-882a-5bb588c5f170@kernel.org>
 <CAO2zrtaGnfH2yYttq4WqeD40Gt9_gerR7sy1szZotbOP+muW5A@mail.gmail.com> <Y7gGUPuoQ/vdGVl6@kroah.com>
In-Reply-To: <Y7gGUPuoQ/vdGVl6@kroah.com>
From:   Hang Zhang <zh.nvgt@gmail.com>
Date:   Fri, 6 Jan 2023 12:39:13 -0500
Message-ID: <CAO2zrtY=zfx4asJ5nuU0RsziTnWV_8vXp7MkbLG_rWGQ5-1YpA@mail.gmail.com>
Subject: Re: [PATCH] tty: vt: add some NULL checks for vc_data
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Jiri Slaby <jirislaby@kernel.org>,
        =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Yangxi Xiang <xyangxi5@gmail.com>,
        Xuezhi Zhang <zhangxuezhi1@coolpad.com>,
        Helge Deller <deller@gmx.de>,
        Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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