Re: [PATCH v5 05/12] drm: revocation check at drm subsystem

On Mon, Apr 29, 2019 at 09:26:07AM +0200, Daniel Vetter wrote:

On Thu, Apr 18, 2019 at 02:27:58PM +0530, Ramalingam C wrote:

...

On every hdcp revocation check request SRM is read from fw file /lib/firmware/display_hdcp_srm.bin

SRM table is parsed and stored at drm_hdcp.c, with functions exported for the services for revocation check from drivers (which implements the HDCP authentication)

This patch handles the HDCP1.4 and 2.2 versions of SRM table.

v2: moved the uAPI to request_firmware_direct() [Daniel]

Signed-off-by: Ramalingam C ramalingam.c@intel.com Suggested-by: Daniel Vetter daniel@ffwll.ch

---

drivers/gpu/drm/Makefile | 2 +- drivers/gpu/drm/drm_hdcp.c | 336 +++++++++++++++++++++++++++++++++

Please add an include stanza for this new file to Documentation/gpu/drm-kms-helpers.rst, somewhere near the other dp/hdcp related sink helpers.

...

drivers/gpu/drm/drm_internal.h | 4 + drivers/gpu/drm/drm_sysfs.c | 2 + include/drm/drm_hdcp.h | 36 ++++ 5 files changed, 379 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_hdcp.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 3d0c75cd687c..fe8400af2426 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -19,7 +19,7 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \ drm_plane.o drm_color_mgmt.o drm_print.o \ drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \ drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \

• drm_atomic_uapi.o
  

• drm_atomic_uapi.o drm_hdcp.o
  
  

drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o drm-$(CONFIG_DRM_VM) += drm_vm.o diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c new file mode 100644 index 000000000000..78b043c8195e --- /dev/null +++ b/drivers/gpu/drm/drm_hdcp.c @@ -0,0 +1,336 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 Intel Corporation.
• • Authors:
• • Ramalingam C ramalingam.c@intel.com
• */

+#include <linux/device.h> +#include <linux/err.h> +#include <linux/gfp.h> +#include <linux/export.h> +#include <linux/slab.h> +#include <linux/firmware.h>

+#include <drm/drm_hdcp.h> +#include <drm/drm_sysfs.h> +#include <drm/drm_print.h> +#include <drm/drm_device.h>

+struct hdcp_srm {

• u8 *srm_buf;
• size_t received_srm_sz;
• u32 revocated_ksv_cnt;
• u8 *revocated_ksv_list;

So not sure this is from the standard or not, but my dictionary says the verb is revoke, not revocate. Please change everywhere from revocated to revoked.

So drm_hdcp_check_ksvs_revoked() for the one exported function. -Daniel

...

• /* Mutex to protect above struct member */
• struct mutex mutex;

+} *srm_data;

+static inline void drm_hdcp_print_ksv(const char *ksv) +{

• DRM_DEBUG("\t%#04x, %#04x, %#04x, %#04x, %#04x\n", *ksv & 0xff,

Why 04? 8 bit only needs 02 I think.

...

•   *(ksv + 1) & 0xff, *(ksv + 2) & 0xff, *(ksv + 3) & 0xff,
  

•   *(ksv + 4) & 0xff);
  

The 0xff is redundant, char is always only 8 bits. You could also simplify the array derefe using ksv[0], ksv[1], ... it's the same expression.

Another one: There's a bit a confusion between const char * and u8* for parsing the buffer. I think it'd be good to standardize on const u8* for everything. I think that should also remove the need for 0xff completely, because no more sign extensions to the full int.

...

+}

+static u32 drm_hdcp_get_revocated_ksv_count(const char *buf, u32 vrls_length) +{

• u32 parsed_bytes = 0, ksv_count = 0, vrl_ksv_cnt, vrl_sz;
• do {

• vrl_ksv_cnt = *buf;
  

• ksv_count += vrl_ksv_cnt;
  

• vrl_sz = (vrl_ksv_cnt * DRM_HDCP_KSV_LEN) + 1;
  

• buf += vrl_sz;
  

• parsed_bytes += vrl_sz;
  

• } while (parsed_bytes < vrls_length);

Hm, if we overflow here (i.e. parsed_bytes > vrls_lenght) then we return garbage, since we already incremented ksv_count. Plus there's no error checking of this. Kernel shouldn't trust root this much, and if the vrls_length and our ksv parsing don't agree, we should abort the srm load. So maybe switch the return value to int, and on error return -EINVAL and abort?

...

• return ksv_count;

+}

+static u32 drm_hdcp_get_revocated_ksvs(const char *buf, u8 *revocated_ksv_list,

• 		       u32 vrls_length)
  

This function here doesn't need to be paranoid, since we already checked the SRM by this point.

...

+{

• u32 parsed_bytes = 0, ksv_count = 0;
• u32 vrl_ksv_cnt, vrl_ksv_sz, vrl_idx = 0;
• do {

• vrl_ksv_cnt = *buf;
  

• vrl_ksv_sz = vrl_ksv_cnt * DRM_HDCP_KSV_LEN;
  

• buf++;
  

• DRM_DEBUG("vrl: %d, Revoked KSVs: %d\n", vrl_idx++,
  

• 	  vrl_ksv_cnt);
  

• memcpy(revocated_ksv_list, buf, vrl_ksv_sz);
  

• ksv_count += vrl_ksv_cnt;
  

• revocated_ksv_list += vrl_ksv_sz;
  

• buf += vrl_ksv_sz;
  

• parsed_bytes += (vrl_ksv_sz + 1);
  

• } while (parsed_bytes < vrls_length);
• return ksv_count;

+}

+static int drm_hdcp_parse_hdcp1_srm(const char *buf, size_t count) +{

• struct hdcp_srm_header *header;
• u32 vrl_length, ksv_count;
• if (count < (sizeof(struct hdcp_srm_header) +

•    DRM_HDCP_1_4_VRL_LENGTH_SIZE + DRM_HDCP_1_4_DCP_SIG_SIZE)) {
  

• DRM_ERROR("Invalid blob length\n");
  

• return -EINVAL;
  

• }
• header = (struct hdcp_srm_header *)buf;
• mutex_lock(&srm_data->mutex);
• DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",

•   header->spec_indicator.srm_id,
  

•   __swab16(header->srm_version), header->srm_gen_no);
  

• WARN_ON(header->spec_indicator.reserved_hi ||

• header->spec_indicator.reserved_lo);
  

• if (header->spec_indicator.srm_id != DRM_HDCP_1_4_SRM_ID) {

• DRM_ERROR("Invalid srm_id\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• buf = buf + sizeof(*header);
• vrl_length = (*buf << 16 | *(buf + 1) << 8 | *(buf + 2));

Maybe a static inline to implement this and use in the hdcp2 version below too. Also I think buf[0], buf[1] is easier to read (as a bikeshed again).

...

• if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||

•    vrl_length < (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
  

• 	  DRM_HDCP_1_4_DCP_SIG_SIZE)) {
  

• DRM_ERROR("Invalid blob length or vrl length\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• /* Length of the all vrls combined */
• vrl_length -= (DRM_HDCP_1_4_VRL_LENGTH_SIZE +

•        DRM_HDCP_1_4_DCP_SIG_SIZE);
  

• if (!vrl_length) {

• DRM_ERROR("No vrl found\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• buf += DRM_HDCP_1_4_VRL_LENGTH_SIZE;
• ksv_count = drm_hdcp_get_revocated_ksv_count(buf, vrl_length);
• if (!ksv_count) {

• DRM_DEBUG("Revocated KSV count is 0\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return count;
  

• }
• kfree(srm_data->revocated_ksv_list);
• srm_data->revocated_ksv_list = kzalloc(ksv_count * DRM_HDCP_KSV_LEN,

• 			       GFP_KERNEL);
  

kcalloc()

...

• if (!srm_data->revocated_ksv_list) {

• DRM_ERROR("Out of Memory\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -ENOMEM;
  

• }
• if (drm_hdcp_get_revocated_ksvs(buf, srm_data->revocated_ksv_list,

• 			vrl_length) != ksv_count) {
  

• srm_data->revocated_ksv_cnt = 0;
  

• kfree(srm_data->revocated_ksv_list);
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• srm_data->revocated_ksv_cnt = ksv_count;
• mutex_unlock(&srm_data->mutex);
• return count;

+}

+static int drm_hdcp_parse_hdcp2_srm(const char *buf, size_t count) +{

• struct hdcp2_srm_header *header;
• u32 vrl_length, ksv_count, ksv_sz;
• mutex_lock(&srm_data->mutex);
• if (count < (sizeof(struct hdcp2_srm_header) +

•    DRM_HDCP_2_VRL_LENGTH_SIZE + DRM_HDCP_2_DCP_SIG_SIZE)) {
  

• DRM_ERROR("Invalid blob length\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• header = (struct hdcp2_srm_header *)buf;
• DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",

•   header->spec_indicator.srm_id,
  

•   __swab16(header->srm_version), header->srm_gen_no);
  

• if (header->spec_indicator.reserved)

• return -EINVAL;
  

• buf = buf + sizeof(*header);
• vrl_length = (*buf << 16 | *(buf + 1) << 8 | *(buf + 2));
• if (count < (sizeof(struct hdcp2_srm_header) + vrl_length) ||

•    vrl_length < (DRM_HDCP_2_VRL_LENGTH_SIZE +
  

•    DRM_HDCP_2_DCP_SIG_SIZE)) {
  

• DRM_ERROR("Invalid blob length or vrl length\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• /* Length of the all vrls combined */
• vrl_length -= (DRM_HDCP_2_VRL_LENGTH_SIZE +

•        DRM_HDCP_2_DCP_SIG_SIZE);
  

• if (!vrl_length) {

• DRM_ERROR("No vrl found\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• buf += DRM_HDCP_2_VRL_LENGTH_SIZE;
• ksv_count = (*buf << 2) | DRM_HDCP_2_KSV_COUNT_2_LSBITS(*(buf + 1));
• if (!ksv_count) {

• DRM_DEBUG("Revocated KSV count is 0\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return count;
  

• }
• kfree(srm_data->revocated_ksv_list);
• srm_data->revocated_ksv_list = kzalloc(ksv_count * DRM_HDCP_KSV_LEN,

• 			       GFP_KERNEL);
  

• if (!srm_data->revocated_ksv_list) {

• DRM_ERROR("Out of Memory\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -ENOMEM;
  

• }
• ksv_sz = ksv_count * DRM_HDCP_KSV_LEN;
• buf += DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ;
• DRM_DEBUG("Revoked KSVs: %d\n", ksv_count);
• memcpy(srm_data->revocated_ksv_list, buf, ksv_sz);
• srm_data->revocated_ksv_cnt = ksv_count;
• mutex_unlock(&srm_data->mutex);
• return count;

+}

+static inline bool is_srm_version_hdcp1(const char *buf) +{

• return ((u8)*buf) == DRM_HDCP_1_4_SRM_ID << 4;

+}

+static inline bool is_srm_version_hdcp2(const char *buf) +{

• return ((u8)*buf) == (DRM_HDCP_2_SRM_ID << 4 |

• 	     DRM_HDCP_2_INDICATOR);
  

+}

+static ssize_t drm_hdcp_srm_update(const char *buf, size_t count) +{

• if (is_srm_version_hdcp1(buf))

• return (ssize_t)drm_hdcp_parse_hdcp1_srm(buf, count);
  

• else if (is_srm_version_hdcp2(buf))

• return (ssize_t)drm_hdcp_parse_hdcp2_srm(buf, count);
  

• return (ssize_t)-EINVAL;

+}

+void drm_hdcp_request_srm(struct drm_device *drm_dev) +{

• char fw_name[36] = "display_hdcp_srm.bin";
• const struct firmware *fw;
• int ret;
• ret = request_firmware_direct(&fw, (const char *)fw_name,

• 		      drm_dev->dev);
  

We need an ack from Matt Roper or someont else from iotg on this.

...

• if (ret < 0)

• goto exit;
  

• if (fw->size && fw->data)

• drm_hdcp_srm_update((const char *)fw->data, fw->size);
  

+exit:

• release_firmware(fw);

+}

+/* Check if any of the KSV is revocated by DCP LLC through SRM table */ +bool drm_hdcp_ksvs_revocated(struct drm_device *drm_dev, u8 *ksvs,

Since comment says "check", maybe put that into the function name? I like to have verbs in function names that do something (instead of simple helper functions to extract a computation to make the code a bit more readable).

Also needs some nice kerneldoc.

...

• 	     u32 ksv_count)
  

+{

• u32 rev_ksv_cnt, cnt, i, j;
• u8 *rev_ksv_list;
• if (!srm_data)

• return false;
  

• drm_hdcp_request_srm(drm_dev);
• mutex_lock(&srm_data->mutex);

I'd have wrapped the locking around the entire function, should simplify things a lot. Or does that anger lockdep when we call into request_firmware()?

...

• rev_ksv_cnt = srm_data->revocated_ksv_cnt;
• rev_ksv_list = srm_data->revocated_ksv_list;
• /* If the Revocated ksv list is empty */
• if (!rev_ksv_cnt || !rev_ksv_list) {

• mutex_unlock(&srm_data->mutex);
  

• return false;
  

• }
• for (cnt = 0; cnt < ksv_count; cnt++) {

• rev_ksv_list = srm_data->revocated_ksv_list;
  

• for (i = 0; i < rev_ksv_cnt; i++) {
  

• 	for (j = 0; j < DRM_HDCP_KSV_LEN; j++)
  

• 		if (*(ksvs + j) != *(rev_ksv_list + j)) {
  

• 			break;
  

• 		} else if (j == (DRM_HDCP_KSV_LEN - 1)) {
  

• 			DRM_DEBUG("Revocated KSV is ");
  

• 			drm_hdcp_print_ksv(ksvs);
  

• 			mutex_unlock(&srm_data->mutex);
  

• 			return true;
  

• 		}
  

• 	/* Move the offset to next KSV in the revocated list */
  

• 	rev_ksv_list += DRM_HDCP_KSV_LEN;
  

• }
  

• /* Iterate to next ksv_offset */
  

• ksvs += DRM_HDCP_KSV_LEN;
  

• }
• mutex_unlock(&srm_data->mutex);
• return false;

+} +EXPORT_SYMBOL_GPL(drm_hdcp_ksvs_revocated);

+int drm_setup_hdcp_srm(struct class *drm_class) +{

• srm_data = kzalloc(sizeof(*srm_data), GFP_KERNEL);
• if (!srm_data)

• return -ENOMEM;
  

• srm_data->srm_buf = kcalloc(DRM_HDCP_SRM_GEN1_MAX_BYTES,

• 		    sizeof(u8), GFP_KERNEL);
  

• if (!srm_data->srm_buf) {

• kfree(srm_data);
  

• return -ENOMEM;
  

• }
• mutex_init(&srm_data->mutex);
• return 0;

+}

+void drm_teardown_hdcp_srm(struct class *drm_class) +{

• if (srm_data) {

• kfree(srm_data->srm_buf);
  

• kfree(srm_data->revocated_ksv_list);
  

• kfree(srm_data);
  

• }

+} diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index d9a483a5fce0..ef3180443870 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -199,3 +199,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent, const struct drm_framebuffer *fb); int drm_framebuffer_debugfs_init(struct drm_minor *minor);

+/* drm_hdcp.c */ +int drm_setup_hdcp_srm(struct class *drm_class); +void drm_teardown_hdcp_srm(struct class *drm_class); diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index ecb7b33002bb..18b1ac442997 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -78,6 +78,7 @@ int drm_sysfs_init(void) }

drm_class->devnode = drm_devnode;

• drm_setup_hdcp_srm(drm_class); return 0;

}

@@ -90,6 +91,7 @@ void drm_sysfs_destroy(void) { if (IS_ERR_OR_NULL(drm_class)) return;

• drm_teardown_hdcp_srm(drm_class); class_remove_file(drm_class, &class_attr_version.attr); class_destroy(drm_class); drm_class = NULL;

diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index f243408ecf26..ff2bcfc1ecef 100644 --- a/include/drm/drm_hdcp.h +++ b/include/drm/drm_hdcp.h @@ -265,4 +265,40 @@ void drm_hdcp2_u32_to_seq_num(u8 seq_num[HDCP_2_2_SEQ_NUM_LEN], u32 val) seq_num[2] = val; }

+#define DRM_HDCP_SRM_GEN1_MAX_BYTES (5 * 1024) +#define DRM_HDCP_1_4_SRM_ID 0x8 +#define DRM_HDCP_1_4_VRL_LENGTH_SIZE 3 +#define DRM_HDCP_1_4_DCP_SIG_SIZE 40

+struct hdcp_srm_header {

• struct {

• u8 reserved_hi:4;
  

• u8 srm_id:4;
  

bitfields in binary structures are heavily discouraged. I think since this is only u8 it should be fine, but explaining that is more work than just having the usual _MASK/_SHIFT #defines like we do with registers.

...

• u8 reserved_lo;
  

• } spec_indicator;

If you wannt __packed, you also need to list this to sub-structs.

...

• u16 srm_version;
• u8 srm_gen_no;

+} __packed;

+#define DRM_HDCP_2_SRM_ID 0x9 +#define DRM_HDCP_2_INDICATOR 0x1 +#define DRM_HDCP_2_VRL_LENGTH_SIZE 3 +#define DRM_HDCP_2_DCP_SIG_SIZE 384 +#define DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ 4

+#define DRM_HDCP_2_KSV_COUNT_2_LSBITS(byte) (((byte) & 0xC) >> 6)

+struct hdcp2_srm_header {

• struct {

• u8 hdcp2_indicator:4;
  

Since hdcp1/2 match except for this I think better to merge them.

...

• u8 srm_id:4;
  

• u8 reserved;
  

• } spec_indicator;
• u16 srm_version;
• u8 srm_gen_no;

+} __packed;

+struct drm_device;

+bool drm_hdcp_ksvs_revocated(struct drm_device *dev, u8 *ksvs, u32 ksv_count); #endif

Cheers, Daniel

...

-- 2.19.1

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch