* [PATCH] drm: Fix HDCP failures when SRM fw is missing
@ 2020-04-14 18:48 ` Sean Paul
0 siblings, 0 replies; 27+ messages in thread
From: Sean Paul @ 2020-04-14 18:48 UTC (permalink / raw)
To: dri-devel, ramalingam.c
Cc: David Airlie, intel-gfx, Sean Paul, stable, Thomas Zimmermann,
Sean Paul
From: Sean Paul <seanpaul@chromium.org>
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.
Fixes: 79643fddd6eb ("drm/hdcp: optimizing the srm handling")
Cc: stable@vger.kernel.org
Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/gpu/drm/drm_hdcp.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
index 7f386adcf872..3c36005d367b 100644
--- a/drivers/gpu/drm/drm_hdcp.c
+++ b/drivers/gpu/drm/drm_hdcp.c
@@ -241,8 +241,10 @@ 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) {
+ ret = 0;
goto exit;
+ }
if (fw->size && fw->data)
ret = drm_hdcp_srm_update(fw->data, fw->size, revoked_ksv_list,
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-14 18:48 ` Sean Paul 0 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-04-14 18:48 UTC (permalink / raw) To: dri-devel, ramalingam.c Cc: intel-gfx, Sean Paul, stable, Sean Paul, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter From: Sean Paul <seanpaul@chromium.org> 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. Fixes: 79643fddd6eb ("drm/hdcp: optimizing the srm handling") Cc: stable@vger.kernel.org Cc: Ramalingam C <ramalingam.c@intel.com> Cc: Sean Paul <sean@poorly.run> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/drm_hdcp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c index 7f386adcf872..3c36005d367b 100644 --- a/drivers/gpu/drm/drm_hdcp.c +++ b/drivers/gpu/drm/drm_hdcp.c @@ -241,8 +241,10 @@ 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) { + ret = 0; goto exit; + } if (fw->size && fw->data) ret = drm_hdcp_srm_update(fw->data, fw->size, revoked_ksv_list, -- Sean Paul, Software Engineer, Google / Chromium OS ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Intel-gfx] [PATCH] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-14 18:48 ` Sean Paul 0 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-04-14 18:48 UTC (permalink / raw) To: dri-devel, ramalingam.c Cc: David Airlie, intel-gfx, Maxime Ripard, Sean Paul, stable, Thomas Zimmermann From: Sean Paul <seanpaul@chromium.org> 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. Fixes: 79643fddd6eb ("drm/hdcp: optimizing the srm handling") Cc: stable@vger.kernel.org Cc: Ramalingam C <ramalingam.c@intel.com> Cc: Sean Paul <sean@poorly.run> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/drm_hdcp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c index 7f386adcf872..3c36005d367b 100644 --- a/drivers/gpu/drm/drm_hdcp.c +++ b/drivers/gpu/drm/drm_hdcp.c @@ -241,8 +241,10 @@ 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) { + ret = 0; goto exit; + } if (fw->size && fw->data) ret = drm_hdcp_srm_update(fw->data, fw->size, revoked_ksv_list, -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2] drm: Fix HDCP failures when SRM fw is missing 2020-04-14 18:48 ` [Intel-gfx] " Sean Paul (?) @ 2020-04-14 19:02 ` Sean Paul -1 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-04-14 19:02 UTC (permalink / raw) To: dri-devel, ramalingam.c Cc: David Airlie, intel-gfx, Sean Paul, stable, Thomas Zimmermann, Sean Paul From: Sean Paul <seanpaul@chromium.org> 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@vger.kernel.org Cc: Ramalingam C <ramalingam.c@intel.com> Cc: Sean Paul <sean@poorly.run> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Sean Paul <seanpaul@chromium.org> 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; + ret = 0; 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; /* 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-14 19:02 ` Sean Paul 0 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-04-14 19:02 UTC (permalink / raw) To: dri-devel, ramalingam.c Cc: intel-gfx, Sean Paul, stable, Sean Paul, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter From: Sean Paul <seanpaul@chromium.org> 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@vger.kernel.org Cc: Ramalingam C <ramalingam.c@intel.com> Cc: Sean Paul <sean@poorly.run> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Sean Paul <seanpaul@chromium.org> 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; + ret = 0; 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; /* 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [Intel-gfx] [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-14 19:02 ` Sean Paul 0 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-04-14 19:02 UTC (permalink / raw) To: dri-devel, ramalingam.c Cc: David Airlie, intel-gfx, Maxime Ripard, Sean Paul, stable, Thomas Zimmermann From: Sean Paul <seanpaul@chromium.org> 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@vger.kernel.org Cc: Ramalingam C <ramalingam.c@intel.com> Cc: Sean Paul <sean@poorly.run> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: dri-devel@lists.freedesktop.org Signed-off-by: Sean Paul <seanpaul@chromium.org> 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; + ret = 0; 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; /* 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2] drm: Fix HDCP failures when SRM fw is missing 2020-04-14 19:02 ` [Intel-gfx] " Sean Paul (?) @ 2020-04-29 13:50 ` Ramalingam C -1 siblings, 0 replies; 27+ messages in thread From: Ramalingam C @ 2020-04-29 13:50 UTC (permalink / raw) To: Sean Paul Cc: dri-devel, David Airlie, intel-gfx, Sean Paul, stable, Thomas Zimmermann On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > From: Sean Paul <seanpaul@chromium.org> > > 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@vger.kernel.org > Cc: Ramalingam C <ramalingam.c@intel.com> > Cc: Sean Paul <sean@poorly.run> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > 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. > + 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) hence pushed a change https://patchwork.freedesktop.org/series/76730/ With these addresed. LGTM. Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > /* 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 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-29 13:50 ` Ramalingam C 0 siblings, 0 replies; 27+ messages in thread From: Ramalingam C @ 2020-04-29 13:50 UTC (permalink / raw) To: Sean Paul Cc: dri-devel, intel-gfx, Sean Paul, stable, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > From: Sean Paul <seanpaul@chromium.org> > > 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@vger.kernel.org > Cc: Ramalingam C <ramalingam.c@intel.com> > Cc: Sean Paul <sean@poorly.run> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > 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. > + 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) hence pushed a change https://patchwork.freedesktop.org/series/76730/ With these addresed. LGTM. Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > /* 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 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-29 13:50 ` Ramalingam C 0 siblings, 0 replies; 27+ messages in thread From: Ramalingam C @ 2020-04-29 13:50 UTC (permalink / raw) To: Sean Paul Cc: dri-devel, David Airlie, intel-gfx, Maxime Ripard, Sean Paul, stable, Thomas Zimmermann On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > From: Sean Paul <seanpaul@chromium.org> > > 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@vger.kernel.org > Cc: Ramalingam C <ramalingam.c@intel.com> > Cc: Sean Paul <sean@poorly.run> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > 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. > + 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) hence pushed a change https://patchwork.freedesktop.org/series/76730/ With these addresed. LGTM. Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > /* 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 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] drm: Fix HDCP failures when SRM fw is missing 2020-04-29 13:50 ` [Intel-gfx] " Ramalingam C (?) @ 2020-04-29 13:58 ` Sean Paul -1 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-04-29 13:58 UTC (permalink / raw) To: Ramalingam C Cc: dri-devel, David Airlie, Intel Graphics Development, Sean Paul, stable, Thomas Zimmermann On Wed, Apr 29, 2020 at 9:50 AM Ramalingam C <ramalingam.c@intel.com> wrote: > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > From: Sean Paul <seanpaul@chromium.org> > > > > 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@vger.kernel.org > > Cc: Ramalingam C <ramalingam.c@intel.com> > > Cc: Sean Paul <sean@poorly.run> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > 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. > > + 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? > hence pushed a > change https://patchwork.freedesktop.org/series/76730/ > > With these addresed. > > LGTM. > > Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > /* 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 > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-29 13:58 ` Sean Paul 0 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-04-29 13:58 UTC (permalink / raw) To: Ramalingam C Cc: dri-devel, Intel Graphics Development, Sean Paul, stable, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter On Wed, Apr 29, 2020 at 9:50 AM Ramalingam C <ramalingam.c@intel.com> wrote: > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > From: Sean Paul <seanpaul@chromium.org> > > > > 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@vger.kernel.org > > Cc: Ramalingam C <ramalingam.c@intel.com> > > Cc: Sean Paul <sean@poorly.run> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > 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. > > + 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? > hence pushed a > change https://patchwork.freedesktop.org/series/76730/ > > With these addresed. > > LGTM. > > Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > /* 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 > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-29 13:58 ` Sean Paul 0 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-04-29 13:58 UTC (permalink / raw) To: Ramalingam C Cc: dri-devel, David Airlie, Intel Graphics Development, Maxime Ripard, Sean Paul, stable, Thomas Zimmermann On Wed, Apr 29, 2020 at 9:50 AM Ramalingam C <ramalingam.c@intel.com> wrote: > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > From: Sean Paul <seanpaul@chromium.org> > > > > 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@vger.kernel.org > > Cc: Ramalingam C <ramalingam.c@intel.com> > > Cc: Sean Paul <sean@poorly.run> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > 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. > > + 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? > hence pushed a > change https://patchwork.freedesktop.org/series/76730/ > > With these addresed. > > LGTM. > > Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > /* 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 > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] drm: Fix HDCP failures when SRM fw is missing 2020-04-29 13:58 ` [Intel-gfx] " Sean Paul (?) @ 2020-04-29 14:22 ` Ramalingam C -1 siblings, 0 replies; 27+ messages in thread From: Ramalingam C @ 2020-04-29 14:22 UTC (permalink / raw) To: Sean Paul Cc: dri-devel, David Airlie, Intel Graphics Development, Sean Paul, stable, Thomas Zimmermann 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@intel.com> wrote: > > > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > 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@vger.kernel.org > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > Cc: Sean Paul <sean@poorly.run> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Maxime Ripard <mripard@kernel.org> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > Cc: David Airlie <airlied@linux.ie> > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > Cc: dri-devel@lists.freedesktop.org > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > 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@intel.com> > > > > > > /* 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 > > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-29 14:22 ` Ramalingam C 0 siblings, 0 replies; 27+ messages in thread From: Ramalingam C @ 2020-04-29 14:22 UTC (permalink / raw) To: Sean Paul Cc: dri-devel, Intel Graphics Development, Sean Paul, stable, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter 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@intel.com> wrote: > > > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > 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@vger.kernel.org > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > Cc: Sean Paul <sean@poorly.run> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Maxime Ripard <mripard@kernel.org> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > Cc: David Airlie <airlied@linux.ie> > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > Cc: dri-devel@lists.freedesktop.org > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > 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@intel.com> > > > > > > /* 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 > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-29 14:22 ` Ramalingam C 0 siblings, 0 replies; 27+ messages in thread From: Ramalingam C @ 2020-04-29 14:22 UTC (permalink / raw) To: Sean Paul Cc: dri-devel, David Airlie, Intel Graphics Development, Maxime Ripard, Sean Paul, stable, Thomas Zimmermann 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@intel.com> wrote: > > > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > 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@vger.kernel.org > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > Cc: Sean Paul <sean@poorly.run> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Maxime Ripard <mripard@kernel.org> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > Cc: David Airlie <airlied@linux.ie> > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > Cc: dri-devel@lists.freedesktop.org > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > 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@intel.com> > > > > > > /* 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 > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] drm: Fix HDCP failures when SRM fw is missing 2020-04-29 14:22 ` [Intel-gfx] " Ramalingam C (?) @ 2020-04-29 14:46 ` Sean Paul -1 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-04-29 14:46 UTC (permalink / raw) To: Ramalingam C Cc: dri-devel, David Airlie, Intel Graphics Development, Sean Paul, stable, Thomas Zimmermann On Wed, Apr 29, 2020 at 10:22 AM Ramalingam C <ramalingam.c@intel.com> wrote: > > 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@intel.com> wrote: > > > > > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > 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@vger.kernel.org > > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > > Cc: Sean Paul <sean@poorly.run> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > Cc: Maxime Ripard <mripard@kernel.org> > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > > Cc: David Airlie <airlied@linux.ie> > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > > Cc: dri-devel@lists.freedesktop.org > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > > > 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. > Ok, thanks for the explanation. This all seems reasonable to me. Looks like this can be applied as-is, right? I'll review the patch you posted so we can ignore the -ve return values. Thanks for the review! Sean > -Ram > > > > > > > hence pushed a > > > change https://patchwork.freedesktop.org/series/76730/ > > > > > > With these addresed. > > > > > > LGTM. > > > > > > Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > > > > > /* 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 > > > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-29 14:46 ` Sean Paul 0 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-04-29 14:46 UTC (permalink / raw) To: Ramalingam C Cc: dri-devel, Intel Graphics Development, Sean Paul, stable, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter On Wed, Apr 29, 2020 at 10:22 AM Ramalingam C <ramalingam.c@intel.com> wrote: > > 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@intel.com> wrote: > > > > > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > 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@vger.kernel.org > > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > > Cc: Sean Paul <sean@poorly.run> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > Cc: Maxime Ripard <mripard@kernel.org> > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > > Cc: David Airlie <airlied@linux.ie> > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > > Cc: dri-devel@lists.freedesktop.org > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > > > 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. > Ok, thanks for the explanation. This all seems reasonable to me. Looks like this can be applied as-is, right? I'll review the patch you posted so we can ignore the -ve return values. Thanks for the review! Sean > -Ram > > > > > > > hence pushed a > > > change https://patchwork.freedesktop.org/series/76730/ > > > > > > With these addresed. > > > > > > LGTM. > > > > > > Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > > > > > /* 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 > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-29 14:46 ` Sean Paul 0 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-04-29 14:46 UTC (permalink / raw) To: Ramalingam C Cc: dri-devel, David Airlie, Intel Graphics Development, Maxime Ripard, Sean Paul, stable, Thomas Zimmermann On Wed, Apr 29, 2020 at 10:22 AM Ramalingam C <ramalingam.c@intel.com> wrote: > > 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@intel.com> wrote: > > > > > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > 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@vger.kernel.org > > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > > Cc: Sean Paul <sean@poorly.run> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > Cc: Maxime Ripard <mripard@kernel.org> > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > > Cc: David Airlie <airlied@linux.ie> > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > > Cc: dri-devel@lists.freedesktop.org > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > > > 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. > Ok, thanks for the explanation. This all seems reasonable to me. Looks like this can be applied as-is, right? I'll review the patch you posted so we can ignore the -ve return values. Thanks for the review! Sean > -Ram > > > > > > > hence pushed a > > > change https://patchwork.freedesktop.org/series/76730/ > > > > > > With these addresed. > > > > > > LGTM. > > > > > > Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > > > > > /* 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 > > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] drm: Fix HDCP failures when SRM fw is missing 2020-04-29 14:46 ` [Intel-gfx] " Sean Paul (?) @ 2020-04-29 16:20 ` Ramalingam C -1 siblings, 0 replies; 27+ messages in thread From: Ramalingam C @ 2020-04-29 16:20 UTC (permalink / raw) To: Sean Paul Cc: dri-devel, David Airlie, Intel Graphics Development, Sean Paul, stable, Thomas Zimmermann On 2020-04-29 at 10:46:29 -0400, Sean Paul wrote: > On Wed, Apr 29, 2020 at 10:22 AM Ramalingam C <ramalingam.c@intel.com> wrote: > > > > 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@intel.com> wrote: > > > > > > > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > 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@vger.kernel.org > > > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > > > Cc: Sean Paul <sean@poorly.run> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > Cc: Maxime Ripard <mripard@kernel.org> > > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > > > Cc: David Airlie <airlied@linux.ie> > > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > > > Cc: dri-devel@lists.freedesktop.org > > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > 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. > > > > Ok, thanks for the explanation. This all seems reasonable to me. > > Looks like this can be applied as-is, right? Yes. Thanks, Ram > I'll review the patch you > posted so we can ignore the -ve return values. > > Thanks for the review! > > Sean > > > -Ram > > > > > > > > > > hence pushed a > > > > change https://patchwork.freedesktop.org/series/76730/ > > > > > > > > With these addresed. > > > > > > > > LGTM. > > > > > > > > Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > > > > > > > /* 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 > > > > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-29 16:20 ` Ramalingam C 0 siblings, 0 replies; 27+ messages in thread From: Ramalingam C @ 2020-04-29 16:20 UTC (permalink / raw) To: Sean Paul Cc: dri-devel, Intel Graphics Development, Sean Paul, stable, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter On 2020-04-29 at 10:46:29 -0400, Sean Paul wrote: > On Wed, Apr 29, 2020 at 10:22 AM Ramalingam C <ramalingam.c@intel.com> wrote: > > > > 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@intel.com> wrote: > > > > > > > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > 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@vger.kernel.org > > > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > > > Cc: Sean Paul <sean@poorly.run> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > Cc: Maxime Ripard <mripard@kernel.org> > > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > > > Cc: David Airlie <airlied@linux.ie> > > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > > > Cc: dri-devel@lists.freedesktop.org > > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > 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. > > > > Ok, thanks for the explanation. This all seems reasonable to me. > > Looks like this can be applied as-is, right? Yes. Thanks, Ram > I'll review the patch you > posted so we can ignore the -ve return values. > > Thanks for the review! > > Sean > > > -Ram > > > > > > > > > > hence pushed a > > > > change https://patchwork.freedesktop.org/series/76730/ > > > > > > > > With these addresed. > > > > > > > > LGTM. > > > > > > > > Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > > > > > > > /* 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 > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-04-29 16:20 ` Ramalingam C 0 siblings, 0 replies; 27+ messages in thread From: Ramalingam C @ 2020-04-29 16:20 UTC (permalink / raw) To: Sean Paul Cc: dri-devel, David Airlie, Intel Graphics Development, Maxime Ripard, Sean Paul, stable, Thomas Zimmermann On 2020-04-29 at 10:46:29 -0400, Sean Paul wrote: > On Wed, Apr 29, 2020 at 10:22 AM Ramalingam C <ramalingam.c@intel.com> wrote: > > > > 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@intel.com> wrote: > > > > > > > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > 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@vger.kernel.org > > > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > > > Cc: Sean Paul <sean@poorly.run> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > Cc: Maxime Ripard <mripard@kernel.org> > > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > > > Cc: David Airlie <airlied@linux.ie> > > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > > > Cc: dri-devel@lists.freedesktop.org > > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > 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. > > > > Ok, thanks for the explanation. This all seems reasonable to me. > > Looks like this can be applied as-is, right? Yes. Thanks, Ram > I'll review the patch you > posted so we can ignore the -ve return values. > > Thanks for the review! > > Sean > > > -Ram > > > > > > > > > > hence pushed a > > > > change https://patchwork.freedesktop.org/series/76730/ > > > > > > > > With these addresed. > > > > > > > > LGTM. > > > > > > > > Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > > > > > > > /* 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 > > > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] drm: Fix HDCP failures when SRM fw is missing 2020-04-29 16:20 ` [Intel-gfx] " Ramalingam C (?) @ 2020-05-05 18:02 ` Sean Paul -1 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-05-05 18:02 UTC (permalink / raw) To: Ramalingam C Cc: dri-devel, David Airlie, Intel Graphics Development, Sean Paul, stable, Thomas Zimmermann On Wed, Apr 29, 2020 at 12:20 PM Ramalingam C <ramalingam.c@intel.com> wrote: > > On 2020-04-29 at 10:46:29 -0400, Sean Paul wrote: > > On Wed, Apr 29, 2020 at 10:22 AM Ramalingam C <ramalingam.c@intel.com> wrote: > > > > > > 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@intel.com> wrote: > > > > > > > > > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > > > 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@vger.kernel.org > > > > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > > > > Cc: Sean Paul <sean@poorly.run> > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > Cc: Maxime Ripard <mripard@kernel.org> > > > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > > > > Cc: David Airlie <airlied@linux.ie> > > > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > > > > Cc: dri-devel@lists.freedesktop.org > > > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > > > 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. > > > > > > > Ok, thanks for the explanation. This all seems reasonable to me. > > > > Looks like this can be applied as-is, right? > Yes. > Applied to drm-misc-fixes Sean > Thanks, > Ram > > > I'll review the patch you > > posted so we can ignore the -ve return values. > > > > Thanks for the review! > > > > Sean > > > > > -Ram > > > > > > > > > > > > > hence pushed a > > > > > change https://patchwork.freedesktop.org/series/76730/ > > > > > > > > > > With these addresed. > > > > > > > > > > LGTM. > > > > > > > > > > Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > > > > > > > > > /* 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 > > > > > > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-05-05 18:02 ` Sean Paul 0 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-05-05 18:02 UTC (permalink / raw) To: Ramalingam C Cc: dri-devel, Intel Graphics Development, Sean Paul, stable, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter On Wed, Apr 29, 2020 at 12:20 PM Ramalingam C <ramalingam.c@intel.com> wrote: > > On 2020-04-29 at 10:46:29 -0400, Sean Paul wrote: > > On Wed, Apr 29, 2020 at 10:22 AM Ramalingam C <ramalingam.c@intel.com> wrote: > > > > > > 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@intel.com> wrote: > > > > > > > > > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > > > 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@vger.kernel.org > > > > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > > > > Cc: Sean Paul <sean@poorly.run> > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > Cc: Maxime Ripard <mripard@kernel.org> > > > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > > > > Cc: David Airlie <airlied@linux.ie> > > > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > > > > Cc: dri-devel@lists.freedesktop.org > > > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > > > 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. > > > > > > > Ok, thanks for the explanation. This all seems reasonable to me. > > > > Looks like this can be applied as-is, right? > Yes. > Applied to drm-misc-fixes Sean > Thanks, > Ram > > > I'll review the patch you > > posted so we can ignore the -ve return values. > > > > Thanks for the review! > > > > Sean > > > > > -Ram > > > > > > > > > > > > > hence pushed a > > > > > change https://patchwork.freedesktop.org/series/76730/ > > > > > > > > > > With these addresed. > > > > > > > > > > LGTM. > > > > > > > > > > Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > > > > > > > > > /* 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 > > > > > > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm: Fix HDCP failures when SRM fw is missing @ 2020-05-05 18:02 ` Sean Paul 0 siblings, 0 replies; 27+ messages in thread From: Sean Paul @ 2020-05-05 18:02 UTC (permalink / raw) To: Ramalingam C Cc: dri-devel, David Airlie, Intel Graphics Development, Maxime Ripard, Sean Paul, stable, Thomas Zimmermann On Wed, Apr 29, 2020 at 12:20 PM Ramalingam C <ramalingam.c@intel.com> wrote: > > On 2020-04-29 at 10:46:29 -0400, Sean Paul wrote: > > On Wed, Apr 29, 2020 at 10:22 AM Ramalingam C <ramalingam.c@intel.com> wrote: > > > > > > 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@intel.com> wrote: > > > > > > > > > > On 2020-04-14 at 15:02:55 -0400, Sean Paul wrote: > > > > > > From: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > > > 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@vger.kernel.org > > > > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > > > > Cc: Sean Paul <sean@poorly.run> > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > Cc: Maxime Ripard <mripard@kernel.org> > > > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > > > > > Cc: David Airlie <airlied@linux.ie> > > > > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > > > > Cc: dri-devel@lists.freedesktop.org > > > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > > > > > > > > > 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. > > > > > > > Ok, thanks for the explanation. This all seems reasonable to me. > > > > Looks like this can be applied as-is, right? > Yes. > Applied to drm-misc-fixes Sean > Thanks, > Ram > > > I'll review the patch you > > posted so we can ignore the -ve return values. > > > > Thanks for the review! > > > > Sean > > > > > -Ram > > > > > > > > > > > > > hence pushed a > > > > > change https://patchwork.freedesktop.org/series/76730/ > > > > > > > > > > With these addresed. > > > > > > > > > > LGTM. > > > > > > > > > > Reviewed-by: Ramalingam C <ramalingam.c@intel.com> > > > > > > > > > > > > /* 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 > > > > > > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 27+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm: Fix HDCP failures when SRM fw is missing (rev2) 2020-04-14 18:48 ` [Intel-gfx] " Sean Paul ` (2 preceding siblings ...) (?) @ 2020-04-15 1:22 ` Patchwork -1 siblings, 0 replies; 27+ messages in thread From: Patchwork @ 2020-04-15 1:22 UTC (permalink / raw) To: Sean Paul; +Cc: intel-gfx == Series Details == Series: drm: Fix HDCP failures when SRM fw is missing (rev2) URL : https://patchwork.freedesktop.org/series/75938/ State : warning == Summary == $ dim checkpatch origin/drm-tip e0a6dea979f6 drm: Fix HDCP failures when SRM fw is missing -:6: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 79643fddd6eb ("drm/hdcp: optimizing the srm handling")' #6: The SRM cleanup in 79643fddd6eb2 ("drm/hdcp: optimizing the srm total: 1 errors, 0 warnings, 0 checks, 21 lines checked _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 27+ messages in thread
* [Intel-gfx] ✗ Fi.CI.DOCS: warning for drm: Fix HDCP failures when SRM fw is missing (rev2) 2020-04-14 18:48 ` [Intel-gfx] " Sean Paul ` (3 preceding siblings ...) (?) @ 2020-04-15 1:39 ` Patchwork -1 siblings, 0 replies; 27+ messages in thread From: Patchwork @ 2020-04-15 1:39 UTC (permalink / raw) To: Sean Paul; +Cc: intel-gfx == Series Details == Series: drm: Fix HDCP failures when SRM fw is missing (rev2) URL : https://patchwork.freedesktop.org/series/75938/ State : warning == Summary == $ make htmldocs 2>&1 > /dev/null | grep i915 /home/cidrm/kernel/Documentation/gpu/i915.rst:610: WARNING: duplicate label gpu/i915:layout, other instance in /home/cidrm/kernel/Documentation/gpu/i915.rst _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 27+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm: Fix HDCP failures when SRM fw is missing (rev2) 2020-04-14 18:48 ` [Intel-gfx] " Sean Paul ` (4 preceding siblings ...) (?) @ 2020-04-15 1:46 ` Patchwork -1 siblings, 0 replies; 27+ messages in thread From: Patchwork @ 2020-04-15 1:46 UTC (permalink / raw) To: Sean Paul; +Cc: intel-gfx == Series Details == Series: drm: Fix HDCP failures when SRM fw is missing (rev2) URL : https://patchwork.freedesktop.org/series/75938/ State : failure == Summary == CI Bug Log - changes from CI_DRM_8298 -> Patchwork_17299 ==================================================== Summary ------- **FAILURE** Serious unknown changes coming with Patchwork_17299 absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_17299, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17299/index.html Possible new issues ------------------- Here are the unknown changes that may have been introduced in Patchwork_17299: ### IGT changes ### #### Possible regressions #### * igt@runner@aborted: - fi-bsw-n3050: NOTRUN -> [FAIL][1] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17299/fi-bsw-n3050/igt@runner@aborted.html Known issues ------------ Here are the changes found in Patchwork_17299 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@i915_selftest@live@gt_lrc: - fi-bsw-n3050: [PASS][2] -> [INCOMPLETE][3] ([i915#1436]) [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/fi-bsw-n3050/igt@i915_selftest@live@gt_lrc.html [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17299/fi-bsw-n3050/igt@i915_selftest@live@gt_lrc.html [i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436 Participating hosts (48 -> 44) ------------------------------ Missing (4): fi-byt-clapper fi-byt-squawks fi-bsw-cyan fi-hsw-4200u Build changes ------------- * CI: CI-20190529 -> None * Linux: CI_DRM_8298 -> Patchwork_17299 CI-20190529: 20190529 CI_DRM_8298: 17f82f0c2857d0b442adbdb62eb44b61d0f5b775 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_5589: 31962324ac86f029e2841e56e97c42cf9d572956 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_17299: e0a6dea979f6de3160fd4579d029c45131e86c69 @ git://anongit.freedesktop.org/gfx-ci/linux == Linux commits == e0a6dea979f6 drm: Fix HDCP failures when SRM fw is missing == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17299/index.html _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-05-05 18:03 UTC | newest] Thread overview: 27+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-04-14 18:48 [PATCH] drm: Fix HDCP failures when SRM fw is missing Sean Paul 2020-04-14 18:48 ` Sean Paul 2020-04-14 18:48 ` [Intel-gfx] " Sean Paul 2020-04-14 19:02 ` [PATCH v2] " Sean Paul 2020-04-14 19:02 ` Sean Paul 2020-04-14 19:02 ` [Intel-gfx] " Sean Paul 2020-04-29 13:50 ` Ramalingam C 2020-04-29 13:50 ` Ramalingam C 2020-04-29 13:50 ` [Intel-gfx] " Ramalingam C 2020-04-29 13:58 ` Sean Paul 2020-04-29 13:58 ` Sean Paul 2020-04-29 13:58 ` [Intel-gfx] " Sean Paul 2020-04-29 14:22 ` Ramalingam C 2020-04-29 14:22 ` Ramalingam C 2020-04-29 14:22 ` [Intel-gfx] " Ramalingam C 2020-04-29 14:46 ` Sean Paul 2020-04-29 14:46 ` Sean Paul 2020-04-29 14:46 ` [Intel-gfx] " Sean Paul 2020-04-29 16:20 ` Ramalingam C 2020-04-29 16:20 ` Ramalingam C 2020-04-29 16:20 ` [Intel-gfx] " Ramalingam C 2020-05-05 18:02 ` Sean Paul 2020-05-05 18:02 ` Sean Paul 2020-05-05 18:02 ` [Intel-gfx] " Sean Paul 2020-04-15 1:22 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm: Fix HDCP failures when SRM fw is missing (rev2) Patchwork 2020-04-15 1:39 ` [Intel-gfx] ✗ Fi.CI.DOCS: " Patchwork 2020-04-15 1:46 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.