[PATCH] drm/msm/dpu: fix stack smashing in dpu_hw_ctl_setup_blendstage

[Abhinav Kumar]

On 2/23/2023 2:08 PM, Dmitry Baryshkov wrote:
> Hi Abhinav,
> 
> On Thu, 23 Feb 2023 at 21:17, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>> Hi Dmitry
>>
>> On 2/23/2023 1:57 AM, Dmitry Baryshkov wrote:
>>> The rewritten dpu_hw_ctl_setup_blendstage() can lightly smash the stack
>>> when setting the SSPP_NONE pipe. However it was unnoticed until the
>>> kernel was tested under AOSP (with some kind of stack protection/check).
>>>
>>> This fixes the following backtrace:
>>>
>>> Unexpected kernel BRK exception at EL1
>>> Internal error: BRK handler: 00000000f20003e8 [#1] PREEMPT SMP
>>> Hardware name: Thundercomm Dragonboard 845c (DT)
>>> pstate: a0400005 (NzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>> pc : dpu_hw_ctl_setup_blendstage+0x26c/0x278 [msm]
>>> lr : _dpu_crtc_blend_setup+0x4b4/0x5a0 [msm]
>>> sp : ffffffc00bdcb720
>>> x29: ffffffc00bdcb720 x28: ffffff8085debac0 x27: 0000000000000002
>>> x26: ffffffd74af18320 x25: ffffff8083af75a0 x24: ffffffc00bdcb878
>>> x23: 0000000000000001 x22: 0000000000000000 x21: ffffff8085a70000
>>> x20: ffffff8083012dc0 x19: 0000000000000001 x18: 0000000000000000
>>> x17: 000000040044ffff x16: 045000f4b5593519 x15: 0000000000000000
>>> x14: 000000000000000b x13: 0000000000000001 x12: 0000000000000000
>>> x11: 0000000000000001 x10: ffffffc00bdcb764 x9 : ffffffd74af06a08
>>> x8 : 0000000000000001 x7 : 0000000000000001 x6 : 0000000000000000
>>> x5 : ffffffc00bdcb878 x4 : 0000000000000002 x3 : ffffffffffffffff
>>> x2 : ffffffc00bdcb878 x1 : 0000000000000000 x0 : 0000000000000002
>>> Call trace:
>>>    dpu_hw_ctl_setup_blendstage+0x26c/0x278 [msm]
>>>    _dpu_crtc_blend_setup+0x4b4/0x5a0 [msm]
>>>    dpu_crtc_atomic_begin+0xd8/0x22c [msm]
>>>    drm_atomic_helper_commit_planes+0x80/0x208 [drm_kms_helper]
>>>    msm_atomic_commit_tail+0x134/0x6f0 [msm]
>>>    commit_tail+0xa4/0x1a4 [drm_kms_helper]
>>>    drm_atomic_helper_commit+0x170/0x184 [drm_kms_helper]
>>>    drm_atomic_commit+0xac/0xe8
>>>    drm_mode_atomic_ioctl+0xbf0/0xdac
>>>    drm_ioctl_kernel+0xc4/0x178
>>>    drm_ioctl+0x2c8/0x608
>>>    __arm64_sys_ioctl+0xa8/0xec
>>>    invoke_syscall+0x44/0x104
>>>    el0_svc_common.constprop.0+0x44/0xec
>>>    do_el0_svc+0x38/0x98
>>>    el0_svc+0x2c/0xb4
>>>    el0t_64_sync_handler+0xb8/0xbc
>>>    el0t_64_sync+0x1a0/0x1a4
>>> Code: 52800016 52800017 52800018 17ffffc7 (d4207d00)
>>>
>>> Fixes: 4488f71f6373 ("drm/msm/dpu: simplify blend configuration")
>>> Reported-by: Amit Pundir <amit.pundir at linaro.org>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>> index b88a2f3724e6..6c53ea560ffa 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>>> @@ -446,7 +446,9 @@ static void dpu_hw_ctl_setup_blendstage(struct dpu_hw_ctl *ctx,
>>>                         * CTL_LAYER has 3-bit field (and extra bits in EXT register),
>>>                         * all EXT registers has 4-bit fields.
>>>                         */
>>> -                     if (cfg->idx == 0) {
>>> +                     if (cfg->idx == -1) {
>>> +                             continue;
>>> +                     } else if (cfg->idx == 0) {
>>>                                mixercfg[0] |= mix << cfg->shift;
>>>                                mixercfg[1] |= ext << cfg->ext_shift;
>>>                        } else {
>>
>> Since I had not reviewed the change which introduced this, had a question.
>>
>> The issue here is because the shift and ext_shift are -1 for NONE and
>> hence the shift causes overflow?
>>
>> If that was the issue shouldnt we protect all such cases?
> 
> This change protects all the cases.
> 
>> So lets say we use SSPP_RGB0, the multirect_index for it will always be
>> -1 as it doesnt support smartDMA. What prevents the same issue from
>> hitting in that case? Because you are only checking for idx and not the
>> shifts.
> 
> Because for the RGB0 / rect-2 the cfg->idx will also be -1 (and
> shift/ext_shift will be 0).
> 
> 

Thanks for confirming, I have understood it now, LGTM

Reviewed-by: Abhinav Kumar <quic_abhinavk at quicinc.com>

Will pick this up for -fixes