[PATCH v2] drm/msm/a6xx+: Don't let IB_SIZE overflow

[Antonino Maniscalco]

On 3/14/25 10:56 PM, Rob Clark wrote:
> On Fri, Mar 14, 2025 at 2:35 PM Antonino Maniscalco
> <antomani103 at gmail.com> wrote:
>>
>> On 3/14/25 10:08 PM, Rob Clark wrote:
>>> On Fri, Mar 14, 2025 at 1:07 PM Akhil P Oommen <quic_akhilpo at quicinc.com> wrote:
>>>>
>>>> On 3/15/2025 12:04 AM, Rob Clark wrote:
>>>>> From: Rob Clark <robdclark at chromium.org>
>>>>>
>>>>> IB_SIZE is only b0..b19.  Starting with a6xx gen3, additional fields
>>>>> were added above the IB_SIZE.  Accidentially setting them can cause
>>>>> badness.  Fix this by properly defining the CP_INDIRECT_BUFFER packet
>>>>> and using the generated builder macro to ensure unintended bits are not
>>>>> set.
>>
>> I wonder if we should be returning -EINVAL from the ioctl when a size
>> larger than max is submitted. I say this because we do a 0 check when
>> submitting which this bug allows to bypass therefore putting a 0 sized
>> CP_INDIRECT_BUFFER pkt in the ring.
>> Fw is inconsistent as to how this is treated (should be a NOP but will
>> sometimes hang) and that is very confusing.
> 
> tbh, I'm not sure I remember why we check for zero.. at least sqe fw
> explicitly handles this case on newer devices.  Maybe it made older
> devices grumpy?
> 

The problem occurs when a CP_INDIRECT_BUFFER with size 0 is encountered, 
say in BV, when BV is disabled.

But yeah I don't feel strongly about this just wanted to point out that 
a check is being bypassed and it does lead to some kind of UB (not sure 
what this does across devices).

> But I wanted to avoid hard-coding (potentially) device specific limits
> in the frontend.  And decided that userspace is allowed to shoot it's
> own foot if it really wants to.
> 
> BR,
> -R
> 
>>>>>
>>>>> v2: add missing type attribute for IB_BASE
>>>>>
>>>>> Reported-by: Connor Abbott <cwabbott0 at gmail.com>
>>>>> Fixes: a83366ef19ea ("drm/msm/a6xx: add A640/A650 to gpulist")
>>>>> Signed-off-by: Rob Clark <robdclark at chromium.org>
>>>>> ---
>>>>> Backport notes, prior to commit ae22a94997b8 ("drm/msm: import A2xx-A4xx
>>>>> XML display registers database"), just open code, ie:
>>>>>
>>>>>      OUT_RING(ring, submit->cmd[i].size & 0xfffff);
>>>>>
>>>>> Prior to commit af66706accdf ("drm/msm/a6xx: Add skeleton A7xx
>>>>> support"), a7xx_submit() did not exist so that hunk can be dropped.
>>>>>
>>>>>    drivers/gpu/drm/msm/adreno/a6xx_gpu.c               | 8 ++++----
>>>>>    drivers/gpu/drm/msm/registers/adreno/adreno_pm4.xml | 7 +++++++
>>>>>    2 files changed, 11 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>>> index d3978cfa3f20..ea52b7d0b212 100644
>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>>> @@ -245,10 +245,10 @@ static void a6xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>>>>>                                 break;
>>>>>                         fallthrough;
>>>>>                 case MSM_SUBMIT_CMD_BUF:
>>>>> -                     OUT_PKT7(ring, CP_INDIRECT_BUFFER_PFE, 3);
>>>>> +                     OUT_PKT7(ring, CP_INDIRECT_BUFFER, 3);
>>>>>                         OUT_RING(ring, lower_32_bits(submit->cmd[i].iova));
>>>>>                         OUT_RING(ring, upper_32_bits(submit->cmd[i].iova));
>>>>> -                     OUT_RING(ring, submit->cmd[i].size);
>>>>> +                     OUT_RING(ring, A5XX_CP_INDIRECT_BUFFER_3_IB_SIZE(submit->cmd[i].size));
>>>>>                         ibs++;
>>>>>                         break;
>>>>>                 }
>>>>> @@ -382,10 +382,10 @@ static void a7xx_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
>>>>>                                 break;
>>>>>                         fallthrough;
>>>>>                 case MSM_SUBMIT_CMD_BUF:
>>>>> -                     OUT_PKT7(ring, CP_INDIRECT_BUFFER_PFE, 3);
>>>>> +                     OUT_PKT7(ring, CP_INDIRECT_BUFFER, 3);
>>>>>                         OUT_RING(ring, lower_32_bits(submit->cmd[i].iova));
>>>>>                         OUT_RING(ring, upper_32_bits(submit->cmd[i].iova));
>>>>> -                     OUT_RING(ring, submit->cmd[i].size);
>>>>> +                     OUT_RING(ring, A5XX_CP_INDIRECT_BUFFER_3_IB_SIZE(submit->cmd[i].size));
>>>>>                         ibs++;
>>>>>                         break;
>>>>>                 }
>>>>> diff --git a/drivers/gpu/drm/msm/registers/adreno/adreno_pm4.xml b/drivers/gpu/drm/msm/registers/adreno/adreno_pm4.xml
>>>>> index 55a35182858c..a71bc6f16cbf 100644
>>>>> --- a/drivers/gpu/drm/msm/registers/adreno/adreno_pm4.xml
>>>>> +++ b/drivers/gpu/drm/msm/registers/adreno/adreno_pm4.xml
>>>>> @@ -2259,5 +2259,12 @@ opcode: CP_LOAD_STATE4 (30) (4 dwords)
>>>>>         </reg32>
>>>>>    </domain>
>>>>>
>>>>> +<domain name="CP_INDIRECT_BUFFER" width="32" varset="chip" prefix="chip" variants="A5XX-">
>>>>> +     <reg64 offset="0" name="IB_BASE" type="address"/>
>>>>> +     <reg32 offset="3" name="3">
>>>>
>>>> Why is the offset 3 here? It looks to me that it doesn't match the code
>>>> above.
>>>
>>> oh, bad copy/pasta.. it should be 2 (dword offset)
>>>
>>> BR,
>>> -R
>>>
>>>> -Akhil.
>>>>
>>>>> +             <bitfield name="IB_SIZE" low="0" high="19"/>
>>>>> +     </reg32>
>>>>> +</domain>
>>>>> +
>>>>>    </database>
>>>>>
>>>>
>>
>> Best regards,
>> --
>> Antonino Maniscalco <antomani103 at gmail.com>


Best regards,
-- 
Antonino Maniscalco <antomani103 at gmail.com>