| Message ID | 20230223095708.3688148-1-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm/dpu: fix stack smashing in dpu_hw_ctl_setup_blendstage | expand |
On 23.02.2023 10:57, 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@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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) { if (cfg->idx == ctl_blend_config[SSPP_NONE][0].idx)? Konrad > + continue; > + } else if (cfg->idx == 0) { > mixercfg[0] |= mix << cfg->shift; > mixercfg[1] |= ext << cfg->ext_shift; > } else { >
On Thu, 23 Feb 2023 at 12:57, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > On 23.02.2023 10:57, 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@linaro.org> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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) { > if (cfg->idx == ctl_blend_config[SSPP_NONE][0].idx)? Why? -1 is simpler and more obvious (and doesn't bind it only to SSPP_NONE). > > Konrad > > + continue; > > + } else if (cfg->idx == 0) { > > mixercfg[0] |= mix << cfg->shift; > > mixercfg[1] |= ext << cfg->ext_shift; > > } else { > >
On 23.02.2023 12:53, Dmitry Baryshkov wrote: > On Thu, 23 Feb 2023 at 12:57, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> >> >> On 23.02.2023 10:57, 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@linaro.org> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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) { >> if (cfg->idx == ctl_blend_config[SSPP_NONE][0].idx)? > > Why? -1 is simpler and more obvious (and doesn't bind it only to SSPP_NONE). Obvious enough to the point of requiring a Fixes? :P But I see your point, it's probably better to leave it as -1 due to its ambiguity. Konrad > >> >> Konrad >>> + continue; >>> + } else if (cfg->idx == 0) { >>> mixercfg[0] |= mix << cfg->shift; >>> mixercfg[1] |= ext << cfg->ext_shift; >>> } else { >>> > > >
On Thu, 23 Feb 2023 at 15:27, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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: Tested-by: Amit Pundir <amit.pundir@linaro.org> > > 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@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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 { > -- > 2.30.2 >
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@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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? 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.
Hi Abhinav, On Thu, 23 Feb 2023 at 21:17, Abhinav Kumar <quic_abhinavk@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@linaro.org> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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).
On 2/23/2023 2:08 PM, Dmitry Baryshkov wrote: > Hi Abhinav, > > On Thu, 23 Feb 2023 at 21:17, Abhinav Kumar <quic_abhinavk@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@linaro.org> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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@quicinc.com> Will pick this up for -fixes
On Thu, 23 Feb 2023 12:57:08 +0300, 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) > > [...] Applied, thanks! [1/1] drm/msm/dpu: fix stack smashing in dpu_hw_ctl_setup_blendstage https://gitlab.freedesktop.org/drm/msm/-/commit/1c1ded39bfb8 Best regards,
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 {
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@linaro.org> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)