Message-ID: <CADnq5_PxMQ_AkBCHXU_YUAMWaPcH-nkOJNGNKnUOJWSTYV6X+A@mail.gmail.com>
Date:   Mon, 4 Nov 2019 12:05:40 -0500
From:   Alex Deucher <alexdeucher@...il.com>
To:     Daniel Vetter <daniel@...ll.ch>
Cc:     Harry Wentland <hwentlan@....com>,
        "Zhou, David(ChunMing)" <David1.Zhou@....com>,
        "Li, Sun peng (Leo)" <Sunpeng.Li@....com>,
        "Wentland, Harry" <Harry.Wentland@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        amd-gfx mailing list <amd-gfx@...ts.freedesktop.org>,
        David Airlie <airlied@...ux.ie>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        Colin Ian King <colin.king@...onical.com>,
        "Lakha, Bhawanpreet" <Bhawanpreet.Lakha@....com>,
        "Koenig, Christian" <Christian.Koenig@....com>
Subject: Re: drm/amd/display: Add HDCP module - static analysis bug report

On Mon, Nov 4, 2019 at 11:55 AM Daniel Vetter <daniel@...ll.ch> wrote:
>
> On Mon, Nov 04, 2019 at 03:23:09PM +0000, Harry Wentland wrote:
> > On 2019-11-04 5:53 a.m., Daniel Vetter wrote:
> > > On Wed, Oct 9, 2019 at 10:58 PM Daniel Vetter <daniel@...ll.ch> wrote:
> > >> On Wed, Oct 9, 2019 at 10:46 PM Lakha, Bhawanpreet
> > >> <Bhawanpreet.Lakha@....com> wrote:
> > >>>
> > >>> I misunderstood and was talking about the ksv validation specifically
> > >>> (usage of drm_hdcp_check_ksvs_revoked()).
> > >>
> > >> Hm for that specifically I think you want to do both, i.e. both
> > >> consult your psp, but also check for revoked ksvs with the core
> > >> helper. At least on some platforms only the core helper might have the
> > >> updated revoke list.
> > >>
> >
> > I think it's an either/or. Either we use an HDCP implementation that's
> > fully running in x86 kernel space (still not sure how that's compliant)
> > or we fully rely on our PSP FW to do what it's designed to do. I don't
> > think it makes sense to mix and match here.
>
> Then you need to somehow tie the revoke list that's in the psp to the
> revoke list update logic we have. That's what we've done for hdcp2 (which
> is similarly to yours implemented in hw). The point is that on linux we
> now have a standard way to get these revoke lists updated/handled.
>
> I guess it wasn't clear how exactly I think you're supposed to combine
> them?

There's no driver sw required at all for our implementation and as far
as I know, HDCP 2.x requires that all of the key revoke handling be
handled in a secure processor rather than than on the host processor,
so I'm not sure how we make use if it.  All the driver sw is
responsible for doing is saving/restoring the potentially updated srm
at suspend/resume/etc.

Alex

> -Daniel
>
>
> >
> > >>> For the defines I will create patches to use drm_hdcp where it is usable.
> > >>
> > >> Thanks a lot. Ime once we have shared definitions it's much easier to
> > >> also share some helpers, where it makes sense.
> > >>
> > >> Aside I think the hdcp code could also use a bit of demidlayering. At
> > >> least I'm not understanding why you add a 2nd abstraction layer for
> > >> i2c/dpcd, dm_helper already has that. That seems like one abstraction
> > >> layer too much.
> > >
> > > I haven't seen anything fly by or in the latest pull request ... you
> > > folks still working on this or more put on the "maybe, probably never"
> > > pile?
> > >
> >
> > Following up with Bhawan.
> >
> > Harry
> >
> > > -Daniel
> > >
> > >
> > >> -Daniel
> > >>
> > >>>
> > >>>
> > >>> Bhawan
> > >>>
> > >>> On 2019-10-09 2:43 p.m., Daniel Vetter wrote:
> > >>>> On Wed, Oct 9, 2019 at 8:23 PM Lakha, Bhawanpreet
> > >>>> <Bhawanpreet.Lakha@....com> wrote:
> > >>>>> Hi,
> > >>>>>
> > >>>>> The reason we don't use drm_hdcp is because our policy is to do hdcp
> > >>>>> verification using PSP/HW (onboard secure processor).
> > >>>> i915 also uses hw to auth, we still use the parts from drm_hdcp ...
> > >>>> Did you actually look at what's in there? It's essentially just shared
> > >>>> defines and data structures from the standard, plus a few minimal
> > >>>> helpers to en/decode some bits. Just from a quick read the entire
> > >>>> patch very much looks like midlayer everywhere design that we
> > >>>> discussed back when DC landed ...
> > >>>> -Daniel
> > >>>>
> > >>>>> Bhawan
> > >>>>>
> > >>>>> On 2019-10-09 12:32 p.m., Daniel Vetter wrote:
> > >>>>>> On Thu, Oct 03, 2019 at 11:08:03PM +0100, Colin Ian King wrote:
> > >>>>>>> Hi,
> > >>>>>>>
> > >>>>>>> Static analysis with Coverity has detected a potential issue with
> > >>>>>>> function validate_bksv in
> > >>>>>>> drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c with recent
> > >>>>>>> commit:
> > >>>>>>>
> > >>>>>>> commit ed9d8e2bcb003ec94658cafe9b1bb3960e2139ec
> > >>>>>>> Author: Bhawanpreet Lakha <Bhawanpreet.Lakha@....com>
> > >>>>>>> Date:   Tue Aug 6 17:52:01 2019 -0400
> > >>>>>>>
> > >>>>>>>       drm/amd/display: Add HDCP module
> > >>>>>> I think the real question here is ... why is this not using drm_hdcp?
> > >>>>>> -Daniel
> > >>>>>>
> > >>>>>>> The analysis is as follows:
> > >>>>>>>
> > >>>>>>>    28 static inline enum mod_hdcp_status validate_bksv(struct mod_hdcp *hdcp)
> > >>>>>>>    29 {
> > >>>>>>>
> > >>>>>>> CID 89852 (#1 of 1): Out-of-bounds read (OVERRUN)
> > >>>>>>>
> > >>>>>>> 1. overrun-local:
> > >>>>>>> Overrunning array of 5 bytes at byte offset 7 by dereferencing pointer
> > >>>>>>> (uint64_t *)hdcp->auth.msg.hdcp1.bksv.
> > >>>>>>>
> > >>>>>>>    30        uint64_t n = *(uint64_t *)hdcp->auth.msg.hdcp1.bksv;
> > >>>>>>>    31        uint8_t count = 0;
> > >>>>>>>    32
> > >>>>>>>    33        while (n) {
> > >>>>>>>    34                count++;
> > >>>>>>>    35                n &= (n - 1);
> > >>>>>>>    36        }
> > >>>>>>>
> > >>>>>>> hdcp->auth.msg.hdcp1.bksv is an array of 5 uint8_t as defined in
> > >>>>>>> drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h as follows:
> > >>>>>>>
> > >>>>>>> struct mod_hdcp_message_hdcp1 {
> > >>>>>>>           uint8_t         an[8];
> > >>>>>>>           uint8_t         aksv[5];
> > >>>>>>>           uint8_t         ainfo;
> > >>>>>>>           uint8_t         bksv[5];
> > >>>>>>>           uint16_t        r0p;
> > >>>>>>>           uint8_t         bcaps;
> > >>>>>>>           uint16_t        bstatus;
> > >>>>>>>           uint8_t         ksvlist[635];
> > >>>>>>>           uint16_t        ksvlist_size;
> > >>>>>>>           uint8_t         vp[20];
> > >>>>>>>
> > >>>>>>>           uint16_t        binfo_dp;
> > >>>>>>> };
> > >>>>>>>
> > >>>>>>> variable n is going to contain the contains of r0p and bcaps. I'm not
> > >>>>>>> sure if that is intentional. If not, then the count is going to be
> > >>>>>>> incorrect if these are non-zero.
> > >>>>>>>
> > >>>>>>> Colin
> > >>>>> _______________________________________________
> > >>>>> dri-devel mailing list
> > >>>>> dri-devel@...ts.freedesktop.org
> > >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >>>>
> > >>>>
> > >>
> > >>
> > >>
> > >> --
> > >> Daniel Vetter
> > >> Software Engineer, Intel Corporation
> > >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Powered by blists - more mailing lists