linux-kernel - Re: [PATCH v4] drm/ssd130x: Allocate buffers in the plane's .atomic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87cz0e4uru.fsf@minerva.mail-host-address-is-not-set>
Date:   Wed, 26 Jul 2023 16:22:45 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Maxime Ripard <mripard@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...il.com>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v4] drm/ssd130x: Allocate buffers in the plane's
 .atomic_check callback

Geert Uytterhoeven <geert@...ux-m68k.org> writes:

Hello Geert,

> Hi Maxime,
>
> On Wed, Jul 26, 2023 at 3:54 PM Maxime Ripard <mripard@...nel.org> wrote:
>> On Wed, Jul 26, 2023 at 02:33:06PM +0200, Geert Uytterhoeven wrote:
>> > > >> Also, Javier pointed me to a discussion you had on IRC about using devm
>> > > >> allocation here. We can't really do that. KMS devices are only freed
>> > > >> once the last userspace application closes its fd to the device file, so
>> > > >> you have an unbounded window during which the driver is still callable
>> > > >> by userspace (and thus can still trigger an atomic commit) but the
>> > > >> buffer would have been freed for a while.
>> > > >
>> > > > It should still be safe for (at least) the data_array buffer. That
>> > > > buffer is only used to store pixels in hardware format, and immediately
>> > > > send them to the hardware.  If this can be called that late, it will
>> > > > fail horribly, as you can no longer talk to the hardware at that point
>> > > > (as ssd130x is an i2c driver, it might actually work; but a DRM driver
>> > > >  that calls devm_platform_ioremap_resource() will crash when writing
>> > > >  to its MMIO registers)?!?
>> > >
>> > > At the very least the SPI driver will fail since the GPIO that is used to
>> > > toggle the D/C pin is allocated with devm_gpiod_get_optional(), but also
>> > > the regulator, backlight device, etc.
>> > >
>> > > But in any case, as mentioned it is only relevant if the data_array buffer
>> > > is allocated at probe time, and from Maxime's explanation is more correct
>> > > to do it in the .atomic_check handler.
>> >
>> > You need (at least) data_array for clear_screen, too, which is called
>> > from .atomic_disable().
>>
>> I'm not sure I get what your concern is?
>>
>> Even if we entirely disable the plane, the state will not have been
>> destroyed yet so you still have at least access to the data_array from
>> the old state.
>
> Currently, clearing the screen is done from the plane's .atomic_disable()
> callback, so the plane's state should be fine.
>
> But IIUIC, DRM allows the user to enable/disable the display without
> creating any plane first, so clearing should be handled in the CRTC's

But it's needed to be clared in this case? Currently the power on/off
is done in the encoder's .atomic_{en,dis}able() but the actual panel
clear is only done for the plane disable as you mentioned.

> .atomic_disable() callback instead? Then, would we still have access
> to valid plane state?
>

If the clear has to be moved to the CRTC atomic enable/disable, then
the driver should be reworked to not use the data_array and instead
just allocate a zero'ed buffer and pass that as you proposed.

But again that's something that could be done later as another patch.

> Thanks!
>
> Gr{oetje,eeting}s,
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat