linux-kernel - Re: [PATCH] drm/msm: UAPI error reporting

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] [day] [month] [year] [list]

Message-ID: <8257e193-df34-4060-af51-0097bdaf11b5@oss.qualcomm.com>
Date: Fri, 13 Dec 2024 17:20:50 +0100
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Rob Clark <robdclark@...il.com>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: dri-devel@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
        freedreno@...ts.freedesktop.org, Rob Clark <robdclark@...omium.org>,
        Sean Paul <sean@...rly.run>, Konrad Dybcio <konradybcio@...nel.org>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/msm: UAPI error reporting

On 13.12.2024 4:55 PM, Rob Clark wrote:
> On Fri, Dec 13, 2024 at 5:11 AM Konrad Dybcio
> <konrad.dybcio@....qualcomm.com> wrote:
>>
>> On 23.11.2024 3:41 AM, Rob Clark wrote:
>>> On Fri, Nov 22, 2024 at 4:19 PM Konrad Dybcio
>>> <konrad.dybcio@....qualcomm.com> wrote:
>>>>
>>>> On 22.11.2024 4:51 PM, Rob Clark wrote:
>>>>> On Fri, Nov 22, 2024 at 4:21 AM Konrad Dybcio
>>>>> <konrad.dybcio@....qualcomm.com> wrote:
>>>>>>
>>>>>> On 21.11.2024 5:48 PM, Rob Clark wrote:
>>>>>>> From: Rob Clark <robdclark@...omium.org>
>>>>>>>
>>>>>>> Debugging incorrect UAPI usage tends to be a bit painful, so add a
>>>>>>> helper macro to make it easier to add debug logging which can be enabled
>>>>>>> at runtime via drm.debug.
>>>>>>>
>>>>>>> Signed-off-by: Rob Clark <robdclark@...omium.org>
>>>>>>> ---
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +/* Helper for returning a UABI error with optional logging which can make
>>>>>>> + * it easier for userspace to understand what it is doing wrong.
>>>>>>> + */
>>>>>>> +#define UERR(err, drm, fmt, ...) \
>>>>>>> +     ({ DRM_DEV_DEBUG_DRIVER((drm)->dev, fmt, ##__VA_ARGS__); -(err); })
>>>>>>> +
>>>>>>>  #define DBG(fmt, ...) DRM_DEBUG_DRIVER(fmt"\n", ##__VA_ARGS__)
>>>>>>>  #define VERB(fmt, ...) if (0) DRM_DEBUG_DRIVER(fmt"\n", ##__VA_ARGS__)
>>>>>>
>>>>>> I'm generally not a fan of adding driver-specific debug prints..
>>>>>>
>>>>>> Maybe that's something that could be pushed to the drm-common layer
>>>>>> or even deeper down the stack?
>>>>>
>>>>> Even if we had something like DRM_DBG_UABI_ERROR() I'd probably still
>>>>> just #define UERR() to be a wrapper for it, since line length/wrapping
>>>>> tends to be a bit of a challenge.  And I have a fairly substantial
>>>>> patch stack on top of this adding sparse/vm_bind support.  (Debugging
>>>>> that was actually the motivation for this patch.)
>>>>
>>>> Alright, let's not get in the way then
>>>>
>>>>> I noticed that xe has something similar, but slightly different shape,
>>>>> in the form of XE_IOCTL_DBG().. but that kinda just moves the line
>>>>> length problem into the if() conditional.  (And doesn't provide the
>>>>> benefit of being able to display the incorrect param.)
>>>>
>>>> Maybe rust comes one day and the lines will start growing vertically ;)
>>>
>>> Rust for the userspace facing rendernode side of the driver, in
>>> particular, would be interesting for me, tbh.  Especially if handle
>>> related rust<->c layers are designed properly.  I've lost track of how
>>> many handle lifetime race condition UAF's I've seen ;-)
>>>
>>> Re-writing entire drivers is a big lift, especially when there is so
>>> much hw+features to enable.  KMS is limited to drm master (generally a
>>> somewhat privileged process), so less of a concern from a security
>>> standpoint.  Much of the GPU side of things is "boring" power related
>>> stuff (suspend/resume/devfreq).  But the rendernode ioctls are open to
>>> any process that can use the GPU in a typical setup.
>>
>> The boring part would benefit greatly from automatic scope exit
>> cleanup.. We've had lots of issues in the past with that (that are
>> hopefully? sorted out now, or should I say, for now)
> 
> Maybe some of the cleanup.h stuff would be useful?

Very possibly.

Though the main issue is that we're juggling two "real" power rails
and two GDSCs that hang off them (with GX being juggled from both AP
and GPU/GMU PoV), and it's simply confusing for the programmer..

I'd rather delay that until some next great cleanup (tm)

Konrad