Re: drm/amd/display: Add HDCP module - static analysis bug report

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Oct 9, 2019 at 10:58 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Oct 9, 2019 at 10:46 PM Lakha, Bhawanpreet
> <Bhawanpreet.Lakha@amd.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.
>
> > 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?

-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@amd.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@amd.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@lists.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