linux-kernel - Re: [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each plane update

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]

Date:   Thu, 13 Jul 2023 15:15:46 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     linux-kernel@...r.kernel.org,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Maxime Ripard <mripard@...nel.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...il.com>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 4/5] drm/ssd130x: Don't allocate buffers on each
 plane update

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

Hello Geert,

> Hi Javier,
>
> On Fri, Jun 9, 2023 at 7:09 PM Javier Martinez Canillas
> <javierm@...hat.com> wrote:
>> The resolutions for these panels are fixed and defined in the Device Tree,
>> so there's no point to allocate the buffers on each plane update and that
>> can just be done once.
>>
>> Let's do the allocation and free on the encoder enable and disable helpers
>> since that's where others initialization and teardown operations are done.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@...hat.com>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@...e.de>
>> ---
>>
>> (no changes since v1)
>
> Thanks for your patch, which is now commit 49d7d581ceaf4cf8
> ("drm/ssd130x: Don't allocate buffers on each plane update") in
> drm-misc/for-linux-next.
>
>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> @@ -701,14 +709,22 @@ static void ssd130x_encoder_helper_atomic_enable(struct drm_encoder *encoder,
>>                 return;
>>
>>         ret = ssd130x_init(ssd130x);
>> -       if (ret) {
>> -               ssd130x_power_off(ssd130x);
>> -               return;
>> -       }
>> +       if (ret)
>> +               goto power_off;
>> +
>> +       ret = ssd130x_buf_alloc(ssd130x);
>
> This appears to be too late, causing a NULL pointer dereference:
>

Thanks for reporting this issue.

> [   59.302761] [<c0303d90>] ssd130x_update_rect.isra.0+0x13c/0x340
> [   59.304231] [<c0304200>]
> ssd130x_primary_plane_helper_atomic_update+0x26c/0x284
> [   59.305716] [<c02f8d54>] drm_atomic_helper_commit_planes+0xfc/0x27c
>

I wonder how this could be too late. I thought that the encoder
.atomic_enable callback would be called before any plane .atomic_update.

> Bailing out from ssd130x_update_rect() when data_array is still NULL
> fixes that.
>

Maybe we can add that with a drm_WARN() ? I still want to understand how
a plane update can happen before an encoder enable.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat