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: <2711de96-fcbe-5611-657a-ab29becd2ff6@gmx.de> <CAKMK7uEOX5n64cjzMt9GRQaS13HFPFyOeqdrkmzc035U5_T8tg@mail.gmail.com>
 <CAKMK7uE7CAXO50JPQ6ziQGGehqfDK2UanBQbfLdUH2RwrwXUvw@mail.gmail.com>
 <CAO2zrtYDcOfvxpG6wPghPnWZVks+NL6N9VaynsBVoX7nHBcxuw@mail.gmail.com>
 <Y7hvhtHeivfsnBtE@phenom.ffwll.local> <CAO2zrtb9H=OWPbrgmqTxQnHqETt-P4K6AxiwdtN7guxUYL2NCw@mail.gmail.com>
 <Y7h++5sU/hGJ/yTS@phenom.ffwll.local> <CAO2zrtaB3GZRSzgWu2kV_jTojwuhtL_kFz0gpzPPVGe9D8kVbQ@mail.gmail.com>
 <Y7iQckF/6EMYVpQt@phenom.ffwll.local> <CAO2zrtY+mMBTLimW6aZhOGFqqw53EVdP0CT8ieW6iVUXevZ7ag@mail.gmail.com>
 <Y7ik3ONV5ba7UXQB@phenom.ffwll.local>
In-Reply-To: <Y7ik3ONV5ba7UXQB@phenom.ffwll.local>
From:   Hang Zhang <zh.nvgt@gmail.com>
Date:   Fri, 6 Jan 2023 18:19:47 -0500
Message-ID: <CAO2zrtYeJg6Zn1SicfBNKBjioO7i07D3ir86+w5v_wVHZYUAWw@mail.gmail.com>
Subject: Re: [PATCH] fbmem: prevent potential use-after-free issues with console_lock()
To:     Hang Zhang <zh.nvgt@gmail.com>, Helge Deller <deller@gmx.de>,
        Javier Martinez Canillas <javierm@redhat.com>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Sam Ravnborg <sam@ravnborg.org>,
        Alex Deucher <alexander.deucher@amd.com>,
        linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
Cc:     Daniel Vetter <daniel@ffwll.ch>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jan 6, 2023 at 5:46 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Jan 06, 2023 at 05:12:57PM -0500, Hang Zhang wrote:
> > On Fri, Jan 6, 2023 at 4:19 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Fri, Jan 06, 2023 at 03:25:14PM -0500, Hang Zhang wrote:
> > > > On Fri, Jan 6, 2023 at 3:05 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > On Fri, Jan 06, 2023 at 02:58:27PM -0500, Hang Zhang wrote:
> > > > > > On Fri, Jan 6, 2023 at 1:59 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > BTW, if this is worthed a fix and the performance of console_lock() is a
> > > > > > major concern, then I think there may be alternative solutions like adding
> > > > > > a lock_fb_info() to the free call chain - if that's better in performance,
> > > > > > or maybe selectively protect the matroxfb ioctl but not vblank ioctl as you
> > > > > > mentioned.
> > > > >
> > > > > Please start out with explaining what kind of bug your checker is seeing,
> > > > > and why. Not how you're trying to fix it. Because I'm pretty sure there
> > > > > isn't a bug, but since I've already spent a pile of time looking at this,
> > > > > I want to make sure.
> > > >
> > > > We are sorry for the inconvenience caused, we'll follow these practices and
> > > > guidelines in the future. Thank you!
> > >
> > > Once more: Please explain what you're static checker is seeing. I want to
> > > understanding this, and I'm hoping at least someone involved in this
> > > static checker can explain what it thinks is going on.
> > >
> > > Thanks, Daniel
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > Thank you for your interest, Daniel. The checker tries first to find
> > the free and
> > use sites of a certain object (in this case "fb_info"), then reason
> > about whether
> > the use can actually happen after the free (e.g., taking into account
> > factors like
> > state set/check, locks, etc.), if so, it will flag a potential
> > use-after-free. As a static
> > checker, is doesn't execute a program or generate a PoC. We then manually
> > review each flagged issue by inspecting all related code. In this
> > case, the checker
> > (and us) are unaware of the lifetime management logic, which may cause
> > problems.
>
> Lifetime management is and absolute basic part in the linux kernel. So if
> your checker flags every free which isn't protected by a lock, then you'll
> creating endless amounts of false positives. Is this really what you're
> doing?
>
> I'm still very confused ...
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Hi, Daniel. Lock is only one of many factors we consider in the checker, so the
actual workflow is certainly more complicated than "mark every free w/o lock".
E.g., we also need to consider the data flow between use and free, the state
check, etc. But as you know, it is very challenging for a static checker to
automatically and accurately reason about everything in the code (automatic
lifetime management analysis can also be tricky for a static analyzer), that's
why we perform a manual review afterward. We will continue working on the
checker to reduce its false alarms and submit higher-quality reports to the
community following your guidelines. Thank you so much for your time!

Best,
Hang