linux-kernel - Re: [PATCH 07/21] drm/msm/dpu: Check CRTC encoders are valid clones

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZtizlTxH-7EBhiSd@intel.com>
Date: Wed, 4 Sep 2024 22:23:01 +0300
From: Ville Syrjälä <ville.syrjala@...ux.intel.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Jessica Zhang <quic_jesszhan@...cinc.com>,
	Rob Clark <robdclark@...il.com>, quic_abhinavk@...cinc.com,
	Sean Paul <sean@...rly.run>,
	Marijn Suijten <marijn.suijten@...ainline.org>,
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>, quic_ebharadw@...cinc.com,
	linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	Rob Clark <robdclark@...omium.org>
Subject: Re: [PATCH 07/21] drm/msm/dpu: Check CRTC encoders are valid clones

On Wed, Sep 04, 2024 at 09:41:23PM +0300, Dmitry Baryshkov wrote:
> On Wed, 4 Sept 2024 at 01:18, Jessica Zhang <quic_jesszhan@...cinc.com> wrote:
> >
> >
> >
> > On 8/30/2024 10:00 AM, Dmitry Baryshkov wrote:
> > > On Thu, Aug 29, 2024 at 01:48:28PM GMT, Jessica Zhang wrote:
> > >> Check that each encoder in the CRTC state's encoder_mask is marked as a
> > >> possible clone for all other encoders in the encoder_mask and that only
> > >> one CRTC is in clone mode at a time
> > >>
> > >> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
> > >> ---
> > >>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 36 +++++++++++++++++++++++++++++++-
> > >>   1 file changed, 35 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > >> index 5ec1b5a38922..bebae365c036 100644
> > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > >> @@ -1,6 +1,6 @@
> > >>   // SPDX-License-Identifier: GPL-2.0-only
> > >>   /*
> > >> - * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
> > >> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > >>    * Copyright (c) 2014-2021 The Linux Foundation. All rights reserved.
> > >>    * Copyright (C) 2013 Red Hat
> > >>    * Author: Rob Clark <robdclark@...il.com>
> > >> @@ -1204,6 +1204,36 @@ static struct msm_display_topology dpu_crtc_get_topology(
> > >>      return topology;
> > >>   }
> > >>
> > >> +static bool dpu_crtc_has_valid_clones(struct drm_crtc *crtc,
> > >> +            struct drm_crtc_state *crtc_state)
> > >> +{
> > >> +    struct drm_encoder *drm_enc;
> > >> +    struct drm_crtc *temp_crtc;
> > >> +    int num_cwb_sessions = 0;
> > >> +
> > >> +    drm_for_each_crtc(temp_crtc, crtc->dev)
> > >> +            if (drm_crtc_in_clone_mode(temp_crtc->state))
> > >
> > > No, get the state from drm_atomic_state. temp_crtc->state might be
> > > irrelevant.
> >
> > Hi Dmitry,
> >
> > Ack.
> >
> > >
> > >> +                    num_cwb_sessions++;
> > >
> > > Even simpler:
> > > if (temp_crtc != crtc && drm_crtc_in_clone_mode(...))
> > >       return false;
> >
> > Ack.
> >
> > >
> > >> +
> > >> +    /*
> > >> +     * Only support a single concurrent writeback session running
> > >> +     * at a time
> > >
> > > If it is not a hardware limitation, please add:
> > > FIXME: support more than one session
> >
> > This is a hardware limitation.
> >
> > >
> > >> +     */
> > >> +    if (num_cwb_sessions > 1)
> > >> +            return false;
> > >> +
> > >> +    drm_for_each_encoder_mask(drm_enc, crtc->dev, crtc_state->encoder_mask) {
> > >> +            if ((crtc_state->encoder_mask & drm_enc->possible_clones) !=
> > >> +                            crtc_state->encoder_mask) {
> > >
> > > Align to opening bracket, please. Granted that other drivers don't
> > > perform this check, is it really necessary? Doesn't
> > > validate_encoder_possible_clones() ensure the same, but during the
> > > encoder registration?
> >
> > The difference here is that validate_encoder_possible_clones() is only
> > called when the drm device is initially registered.
> >
> > The check here is to make sure that the encoders userspace is proposing
> > to be cloned are actually possible clones of each other. This might not
> > be necessary for drivers where all encoders are all possible clones of
> > each other. But for MSM (and CWB), real-time display encoders can only
> > be clones of writeback (and vice versa).
> 
> I had the feeling that encoder_mask should already take care of that,
> but it seems I was wrong.
> Please extract this piece as a generic helper. I think it should be
> called from the generic atomic_check() codepath.

Yeah, if we are semi-assured that drivers aren't screwing up those
bitmasks anymore we could shove the cloning checks into
drm_atomic_helper_check_modeset(). It already checks possible_crtcs.
We could then throw out the equavalent code from i915 as well...

Are there decent IGTs to make sure the kernel properly rejects
illegal cloning configurations?

-- 
Ville Syrjälä
Intel