On 2020-04-29 at 09:58:16 -0400, Sean Paul wrote:
> On Wed, Apr 29, 2020 at 9:50 AM Ramalingam C <ramalingam.c@xxxxxxxxx> wrote:
> >
> > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote:
> > > From: Sean Paul <seanpaul@xxxxxxxxxxxx>
> > >
> > > The SRM cleanup in 79643fddd6eb2 ("drm/hdcp: optimizing the srm
> > > handling") inadvertently altered the behavior of HDCP auth when
> > > the SRM firmware is missing. Before that patch, missing SRM was
> > > interpreted as the device having no revoked keys. With that patch,
> > > if the SRM fw file is missing we reject _all_ keys.
> > >
> > > This patch fixes that regression by returning success if the file
> > > cannot be found. It also checks the return value from request_srm such
> > > that we won't end up trying to parse the ksv list if there is an error
> > > fetching it.
> > >
> > > Fixes: 79643fddd6eb ("drm/hdcp: optimizing the srm handling")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Cc: Ramalingam C <ramalingam.c@xxxxxxxxx>
> > > Cc: Sean Paul <sean@xxxxxxxxxx>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > > Cc: David Airlie <airlied@xxxxxxxx>
> > > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> > >
> > > Changes in v2:
> > > -Noticed a couple other things to clean up
> > > ---
> > >
> > > Sorry for the quick rev, noticed a couple other loose ends that should
> > > be cleaned up.
> > >
> > > drivers/gpu/drm/drm_hdcp.c | 8 +++++++-
> > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> > > index 7f386adcf872..910108ccaae1 100644
> > > --- a/drivers/gpu/drm/drm_hdcp.c
> > > +++ b/drivers/gpu/drm/drm_hdcp.c
> > > @@ -241,8 +241,12 @@ static int drm_hdcp_request_srm(struct drm_device *drm_dev,
> > >
> > > ret = request_firmware_direct(&fw, (const char *)fw_name,
> > > drm_dev->dev);
> > > - if (ret < 0)
> > > + if (ret < 0) {
> > > + *revoked_ksv_cnt = 0;
> > > + *revoked_ksv_list = NULL;
> > These two variables are already initialized by the caller.
>
> Right now it is, but that's not guaranteed. In the ret == 0 case, it's
> pretty common for a caller to assume the called function has
> validated/assigned all the function output.
Ok.
>
> > > + ret = 0;
> > Missing of this should have been caught by CI. May be CI system always
> > having the SRM file from previous execution. Never been removed. IGT
> > need a fix to clean the prior SRM files before execution.
> >
> > CI fix shouldn't block this fix.
> > > goto exit;
> > > + }
> > >
> > > if (fw->size && fw->data)
> > > ret = drm_hdcp_srm_update(fw->data, fw->size, revoked_ksv_list,
> > > @@ -287,6 +291,8 @@ int drm_hdcp_check_ksvs_revoked(struct drm_device *drm_dev, u8 *ksvs,
> > >
> > > ret = drm_hdcp_request_srm(drm_dev, &revoked_ksv_list,
> > > &revoked_ksv_cnt);
> > > + if (ret)
> > > + return ret;
> > This error code also shouldn't effect the caller(i915)
>
> Why not? I'd assume an invalid SRM revocation list should probably be
> treated as failure?
IMHO invalid SRM revocation need not be treated as HDCP authentication
failure.
First of all SRM need not supplied by all players. and incase, supplied
SRM is not as per the spec, then we dont have any list of revoked ID.
with this I dont think we need to fail the HDCP authentication. Until we
have valid list of revoked IDs from SRM, and the receiver ID is matching
to one of the revoked IDs, I wouldn't want to fail the HDCP
authentication.
-Ram
>
>
> > hence pushed a
> > change https://patchwork.freedesktop.org/series/76730/
> >
> > With these addresed.
> >
> > LGTM.
> >
> > Reviewed-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> > >
> > > /* revoked_ksv_cnt will be zero when above function failed */
> > > for (i = 0; i < revoked_ksv_cnt; i++)
> > > --
> > > Sean Paul, Software Engineer, Google / Chromium OS
> > >
