| Message ID | 20171130030907.26848-2-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 2017-11-29 22:08:56, Sean Paul wrote: > This patch adds a new optional connector property to allow userspace to enable > protection over the content it is displaying. This will typically be implemented > by the driver using HDCP. > > The property is a tri-state with the following values: > - OFF: Self explanatory, no content protection > - DESIRED: Userspace requests that the driver enable protection > - ENABLED: Once the driver has authenticated the link, it sets this value > > The driver is responsible for downgrading ENABLED to DESIRED if the link becomes > unprotected. The driver should also maintain the desiredness of protection > across hotplug/dpms/suspend. Why would user of the machine want this to be something else than 'OFF'? If kernel implements this, will it mean hardware vendors will have to prevent user from updating kernel on machines they own? If this is merged, does it open kernel developers to DMCA threats if they try to change it? Pavel
On Tue, Dec 05, 2017 at 11:28:40AM +0100, Pavel Machek wrote: > On Wed 2017-11-29 22:08:56, Sean Paul wrote: > > This patch adds a new optional connector property to allow userspace to enable > > protection over the content it is displaying. This will typically be implemented > > by the driver using HDCP. > > > > The property is a tri-state with the following values: > > - OFF: Self explanatory, no content protection > > - DESIRED: Userspace requests that the driver enable protection > > - ENABLED: Once the driver has authenticated the link, it sets this value > > > > The driver is responsible for downgrading ENABLED to DESIRED if the link becomes > > unprotected. The driver should also maintain the desiredness of protection > > across hotplug/dpms/suspend. > > Why would user of the machine want this to be something else than > 'OFF'? > > If kernel implements this, will it mean hardware vendors will have to > prevent user from updating kernel on machines they own? > > If this is merged, does it open kernel developers to DMCA threats if > they try to change it? Because this just implements one part of the content protection scheme. This only gives you an option to enable HDCP (aka encryption, it's really nothing else) on the cable. Just because it has Content Protection in the name does _not_ mean it is (stand-alone) an effective nor complete content protection scheme. It's simply encrypting data, that's all. If you want to actually lock down a machine to implement content protection, then you need secure boot without unlockable boot-loader and a pile more bits in userspace. If you do all that, only then do you have full content protection. And yes, then you don't really own the machine fully, and I think users who are concerned with being able to update their kernels and be able to exercise their software freedoms already know to avoid such locked down systems. So yeah it would be better to call this the "HDMI/DP cable encryption support", but well, it's not what it's called really. -Daniel
On Tue 2017-12-05 11:45:38, Daniel Vetter wrote: > On Tue, Dec 05, 2017 at 11:28:40AM +0100, Pavel Machek wrote: > > On Wed 2017-11-29 22:08:56, Sean Paul wrote: > > > This patch adds a new optional connector property to allow userspace to enable > > > protection over the content it is displaying. This will typically be implemented > > > by the driver using HDCP. > > > > > > The property is a tri-state with the following values: > > > - OFF: Self explanatory, no content protection > > > - DESIRED: Userspace requests that the driver enable protection > > > - ENABLED: Once the driver has authenticated the link, it sets this value > > > > > > The driver is responsible for downgrading ENABLED to DESIRED if the link becomes > > > unprotected. The driver should also maintain the desiredness of protection > > > across hotplug/dpms/suspend. > > > > Why would user of the machine want this to be something else than > > 'OFF'? > > > > If kernel implements this, will it mean hardware vendors will have to > > prevent user from updating kernel on machines they own? > > > > If this is merged, does it open kernel developers to DMCA threats if > > they try to change it? > > Because this just implements one part of the content protection scheme. > This only gives you an option to enable HDCP (aka encryption, it's really > nothing else) on the cable. Just because it has Content Protection in the > name does _not_ mean it is (stand-alone) an effective nor complete content > protection scheme. It's simply encrypting data, that's all. Yep. So my first question was: why would user of the machine ever want encryption "ENABLED" or "DESIRED"? Could you answer it? > If you want to actually lock down a machine to implement content > protection, then you need secure boot without unlockable boot-loader and a > pile more bits in userspace. If you do all that, only then do you have > full content protection. And yes, then you don't really own the machine > fully, and I think users who are concerned with being able to update > their Yes, so... This patch makes it more likely to see machines with locked down kernels, preventing developers from working with systems their own, running hardware. That is evil, and direct threat to Free software movement. Users compiling their own kernels get no benefit from it. Actually it looks like this only benefits Intel and Disney. We don't want that. > kernels and be able to exercise their software freedoms already know to > avoid such locked down systems. > > So yeah it would be better to call this the "HDMI/DP cable encryption > support", but well, it's not what it's called really. Well, it does not belong in kernel, no matter what is the name. Pavel
On Tue, Dec 5, 2017 at 12:34 PM, Pavel Machek <pavel@ucw.cz> wrote: > On Tue 2017-12-05 11:45:38, Daniel Vetter wrote: >> On Tue, Dec 05, 2017 at 11:28:40AM +0100, Pavel Machek wrote: >> > On Wed 2017-11-29 22:08:56, Sean Paul wrote: >> > > This patch adds a new optional connector property to allow userspace to enable >> > > protection over the content it is displaying. This will typically be implemented >> > > by the driver using HDCP. >> > > >> > > The property is a tri-state with the following values: >> > > - OFF: Self explanatory, no content protection >> > > - DESIRED: Userspace requests that the driver enable protection >> > > - ENABLED: Once the driver has authenticated the link, it sets this value >> > > >> > > The driver is responsible for downgrading ENABLED to DESIRED if the link becomes >> > > unprotected. The driver should also maintain the desiredness of protection >> > > across hotplug/dpms/suspend. >> > >> > Why would user of the machine want this to be something else than >> > 'OFF'? >> > >> > If kernel implements this, will it mean hardware vendors will have to >> > prevent user from updating kernel on machines they own? >> > >> > If this is merged, does it open kernel developers to DMCA threats if >> > they try to change it? >> >> Because this just implements one part of the content protection scheme. >> This only gives you an option to enable HDCP (aka encryption, it's really >> nothing else) on the cable. Just because it has Content Protection in the >> name does _not_ mean it is (stand-alone) an effective nor complete content >> protection scheme. It's simply encrypting data, that's all. > > Yep. So my first question was: why would user of the machine ever want > encryption "ENABLED" or "DESIRED"? Could you answer it? How about for sensitive video streams in government offices where you want to avoid a spy potentially tapping the cable to see the video stream? > >> If you want to actually lock down a machine to implement content >> protection, then you need secure boot without unlockable boot-loader and a >> pile more bits in userspace. If you do all that, only then do you have >> full content protection. And yes, then you don't really own the machine >> fully, and I think users who are concerned with being able to update >> their > > Yes, so... This patch makes it more likely to see machines with locked > down kernels, preventing developers from working with systems their > own, running hardware. That is evil, and direct threat to Free > software movement. > > Users compiling their own kernels get no benefit from it. Actually it > looks like this only benefits Intel and Disney. We don't want that. And just about every SoC manufacturer and google and amazon and a ton of other companies and organizations. Who gets to decide who's benefit gets taken into account? > >> kernels and be able to exercise their software freedoms already know to >> avoid such locked down systems. >> >> So yeah it would be better to call this the "HDMI/DP cable encryption >> support", but well, it's not what it's called really. > > Well, it does not belong in kernel, no matter what is the name. Should we remove support for encrypted file systems and encrypted virtual machines? Just like them the option is there is you want to use it. If you don't want to, you don't have to. Alex
Hi! > >> > Why would user of the machine want this to be something else than > >> > 'OFF'? > >> > > >> > If kernel implements this, will it mean hardware vendors will have to > >> > prevent user from updating kernel on machines they own? > >> > > >> > If this is merged, does it open kernel developers to DMCA threats if > >> > they try to change it? > >> > >> Because this just implements one part of the content protection scheme. > >> This only gives you an option to enable HDCP (aka encryption, it's really > >> nothing else) on the cable. Just because it has Content Protection in the > >> name does _not_ mean it is (stand-alone) an effective nor complete content > >> protection scheme. It's simply encrypting data, that's all. > > > > Yep. So my first question was: why would user of the machine ever want > > encryption "ENABLED" or "DESIRED"? Could you answer it? > > How about for sensitive video streams in government offices where you > want to avoid a spy potentially tapping the cable to see the video > stream? Except that spies already have the keys, as every monitor manufacturer has them? > >> kernels and be able to exercise their software freedoms already know to > >> avoid such locked down systems. > >> > >> So yeah it would be better to call this the "HDMI/DP cable encryption > >> support", but well, it's not what it's called really. > > > > Well, it does not belong in kernel, no matter what is the name. > > Should we remove support for encrypted file systems and encrypted > virtual machines? Just like them the option is there is you want to > use it. If you don't want to, you don't have to. Encrypted file systems benefit users. Encrypted video is designed to work against users. In particular, users don't have encryption keys for video they generate. I'd have nothing against feature that would let users encrypt video with keys they control. Pavel
On Tue, Dec 5, 2017 at 12:34 PM, Pavel Machek <pavel@ucw.cz> wrote: > On Tue 2017-12-05 11:45:38, Daniel Vetter wrote: >> On Tue, Dec 05, 2017 at 11:28:40AM +0100, Pavel Machek wrote: >> > On Wed 2017-11-29 22:08:56, Sean Paul wrote: >> > > This patch adds a new optional connector property to allow userspace to enable >> > > protection over the content it is displaying. This will typically be implemented >> > > by the driver using HDCP. >> > > >> > > The property is a tri-state with the following values: >> > > - OFF: Self explanatory, no content protection >> > > - DESIRED: Userspace requests that the driver enable protection >> > > - ENABLED: Once the driver has authenticated the link, it sets this value >> > > >> > > The driver is responsible for downgrading ENABLED to DESIRED if the link becomes >> > > unprotected. The driver should also maintain the desiredness of protection >> > > across hotplug/dpms/suspend. >> > >> > Why would user of the machine want this to be something else than >> > 'OFF'? >> > >> > If kernel implements this, will it mean hardware vendors will have to >> > prevent user from updating kernel on machines they own? >> > >> > If this is merged, does it open kernel developers to DMCA threats if >> > they try to change it? >> >> Because this just implements one part of the content protection scheme. >> This only gives you an option to enable HDCP (aka encryption, it's really >> nothing else) on the cable. Just because it has Content Protection in the >> name does _not_ mean it is (stand-alone) an effective nor complete content >> protection scheme. It's simply encrypting data, that's all. > > Yep. So my first question was: why would user of the machine ever want > encryption "ENABLED" or "DESIRED"? Could you answer it? > Sure. We have a lot of Chrome OS users who would really like to enjoy premium hd content on their tvs. >> If you want to actually lock down a machine to implement content >> protection, then you need secure boot without unlockable boot-loader and a >> pile more bits in userspace. If you do all that, only then do you have >> full content protection. And yes, then you don't really own the machine >> fully, and I think users who are concerned with being able to update >> their > > Yes, so... This patch makes it more likely to see machines with locked > down kernels, preventing developers from working with systems their > own, running hardware. That is evil, and direct threat to Free > software movement. > > Users compiling their own kernels get no benefit from it. Actually it > looks like this only benefits Intel and Disney. We don't want that. > Major citation needed here. Did you actually read the code? If you did, you would realize that the feature is already latent in your computer. This patchset merely exposes how that hardware can be enabled to encrypt your video link. Would you have the same problems with a new whizzbang video encoding format, or is it just the "Content Protection" name? Perhaps you'd prefer this feature was implemented in Intel's firmware, or a userspace blob? It wouldn't change the fact that those registers exist and _can_ be used for HDCP, it's just that now you know about it. Having all of the code in the open allows users to see what is happening with their hardware, how is this a bad thing? Sean >> kernels and be able to exercise their software freedoms already know to >> avoid such locked down systems. >> >> So yeah it would be better to call this the "HDMI/DP cable encryption >> support", but well, it's not what it's called really. > > Well, it does not belong in kernel, no matter what is the name. > > Pavel > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Pavel, On 5 December 2017 at 17:34, Pavel Machek <pavel@ucw.cz> wrote: > Yes, so... This patch makes it more likely to see machines with locked > down kernels, preventing developers from working with systems their > own, running hardware. That is evil, and direct threat to Free > software movement. > > Users compiling their own kernels get no benefit from it. Actually it > looks like this only benefits Intel and Disney. We don't want that. With all due respect, you can't claim to speak for the entire kernel and FLOSS community of users and developers. The feature is optional: it does not enforce additional constraints on users, but exposes additional functionality already present in hardware, for those who wish to opt in to it. Those who wish to avoid it can do so, by simply not making active use of it. Cheers, Daniel
> If you want to actually lock down a machine to implement content > protection, then you need secure boot without unlockable boot-loader and a > pile more bits in userspace. So let me take my Intel hat off for a moment. The upstream policy has always been that we don't merge things which don't have an open usable user space. Is the HDCP encryption feature useful on its own ? What do users get from it ? If this is just an enabler for a lump of binary stuff in ChromeOS then I don't think it belongs, if it is useful standalone then it seems it does belong ? Alan
> How about for sensitive video streams in government offices where you > want to avoid a spy potentially tapping the cable to see the video > stream? Last time I checked HDCP did not meet government security requirements - which is hardly surprising since you can buy $10 boxes from China to de-hdcp video streams 8) Alan
On Thu, Dec 07, 2017 at 02:30:52PM +0000, Alan Cox wrote: > > If you want to actually lock down a machine to implement content > > protection, then you need secure boot without unlockable boot-loader and a > > pile more bits in userspace. > > So let me take my Intel hat off for a moment. > > The upstream policy has always been that we don't merge things which > don't have an open usable user space. Is the HDCP encryption feature > useful on its own ? What do users get from it ? > > If this is just an enabler for a lump of binary stuff in ChromeOS then I > don't think it belongs, if it is useful standalone then it seems it does > belong ? The cros side is ofc all open source. dri-devel is extremely strict with not taking anything that doesn't fullfil this requirement, probably more strict than anyone else. Sean has the link in the cover letter of his patch series. For more context, here's our documented expectations about the userspace side of any uapi addition to drm: https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements Cheers, Daniel
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 37445d50816a..2212793eefa6 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1185,6 +1185,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, state->picture_aspect_ratio = val; } else if (property == connector->scaling_mode_property) { state->scaling_mode = val; + } else if (property == connector->content_protection_property) { + if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) { + DRM_DEBUG_KMS("only drivers can set CP Enabled\n"); + return -EINVAL; + } + state->content_protection = val; } else if (connector->funcs->atomic_set_property) { return connector->funcs->atomic_set_property(connector, state, property, val); @@ -1264,6 +1270,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->picture_aspect_ratio; } else if (property == connector->scaling_mode_property) { *val = state->scaling_mode; + } else if (property == connector->content_protection_property) { + *val = state->content_protection; } else if (connector->funcs->atomic_get_property) { return connector->funcs->atomic_get_property(connector, state, property, val); diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 704fc8934616..de2345a4a125 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -698,6 +698,13 @@ static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = { DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name, drm_tv_subconnector_enum_list) +static struct drm_prop_enum_list drm_cp_enum_list[] = { + { DRM_MODE_CONTENT_PROTECTION_OFF, "Off" }, + { DRM_MODE_CONTENT_PROTECTION_DESIRED, "Desired" }, + { DRM_MODE_CONTENT_PROTECTION_ENABLED, "Enabled" }, +}; +DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list) + /** * DOC: standard connector properties * @@ -1046,6 +1053,42 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector, } EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property); +/** + * drm_connector_attach_content_protection_property - attach content protection + * property + * + * @connector: connector to attach CP property on. + * + * This is used to add support for content protection on select connectors. + * Content Protection is intentionally vague to allow for different underlying + * technologies, however it is most implemented by HDCP. + * + * The content protection will be set to &drm_connector_state.content_protection + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_connector_attach_content_protection_property( + struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + struct drm_property *prop; + + prop = drm_property_create_enum(dev, 0, "Content Protection", + drm_cp_enum_list, + ARRAY_SIZE(drm_cp_enum_list)); + if (!prop) + return -ENOMEM; + + drm_object_attach_property(&connector->base, prop, + DRM_MODE_CONTENT_PROTECTION_OFF); + + connector->content_protection_property = prop; + + return 0; +} +EXPORT_SYMBOL(drm_connector_attach_content_protection_property); + /** * drm_mode_create_aspect_ratio_property - create aspect ratio property * @dev: DRM device diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 1c5b5ce1fd7f..e8e15756dc59 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -21,6 +21,7 @@ #include <drm/drm_sysfs.h> #include <drm/drmP.h> #include "drm_internal.h" +#include "drm_crtc_internal.h" #define to_drm_minor(d) dev_get_drvdata(d) #define to_drm_connector(d) dev_get_drvdata(d) @@ -229,16 +230,44 @@ static ssize_t modes_show(struct device *device, return written; } +static ssize_t content_protection_show(struct device *device, + struct device_attribute *attr, char *buf) +{ + struct drm_connector *connector = to_drm_connector(device); + struct drm_device *dev = connector->dev; + struct drm_property *prop; + uint64_t cp; + int ret; + + drm_modeset_lock_all(dev); + + prop = connector->content_protection_property; + if (!prop) { + drm_modeset_unlock_all(dev); + return 0; + } + + ret = drm_object_property_get_value(&connector->base, prop, &cp); + drm_modeset_unlock_all(dev); + if (ret) + return 0; + + return snprintf(buf, PAGE_SIZE, "%s\n", + drm_get_content_protection_name((int)cp)); +} + static DEVICE_ATTR_RW(status); static DEVICE_ATTR_RO(enabled); static DEVICE_ATTR_RO(dpms); static DEVICE_ATTR_RO(modes); +static DEVICE_ATTR_RO(content_protection); static struct attribute *connector_dev_attrs[] = { &dev_attr_status.attr, &dev_attr_enabled.attr, &dev_attr_dpms.attr, &dev_attr_modes.attr, + &dev_attr_content_protection.attr, NULL }; diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 1543212b0449..ec6ea3953002 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -375,6 +375,12 @@ struct drm_connector_state { * upscaling, mostly used for built-in panels. */ unsigned int scaling_mode; + + /** + * @content_protection: Connector property to request content + * protection. This is most commonly used for HDCP. + */ + unsigned int content_protection; }; /** @@ -722,6 +728,7 @@ struct drm_cmdline_mode { * @tile_h_size: horizontal size of this tile. * @tile_v_size: vertical size of this tile. * @scaling_mode_property: Optional atomic property to control the upscaling. + * @content_protection_property: Optional property to control content protection * * Each connector may be connected to one or more CRTCs, or may be clonable by * another connector if they can share a CRTC. Each connector also has a specific @@ -812,6 +819,12 @@ struct drm_connector { struct drm_property *scaling_mode_property; + /** + * @content_protection_property: DRM ENUM property for content + * protection + */ + struct drm_property *content_protection_property; + /** * @path_blob_ptr: * @@ -1012,6 +1025,7 @@ const char *drm_get_dvi_i_subconnector_name(int val); const char *drm_get_dvi_i_select_name(int val); const char *drm_get_tv_subconnector_name(int val); const char *drm_get_tv_select_name(int val); +const char *drm_get_content_protection_name(int val); int drm_mode_create_dvi_i_properties(struct drm_device *dev); int drm_mode_create_tv_properties(struct drm_device *dev, @@ -1020,6 +1034,8 @@ int drm_mode_create_tv_properties(struct drm_device *dev, int drm_mode_create_scaling_mode_property(struct drm_device *dev); int drm_connector_attach_scaling_mode_property(struct drm_connector *connector, u32 scaling_mode_mask); +int drm_connector_attach_content_protection_property( + struct drm_connector *connector); int drm_mode_create_aspect_ratio_property(struct drm_device *dev); int drm_mode_create_suggested_offset_properties(struct drm_device *dev); diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 5597a87154e5..03f4d22305c2 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -173,6 +173,10 @@ extern "C" { DRM_MODE_REFLECT_X | \ DRM_MODE_REFLECT_Y) +/* Content Protection Flags */ +#define DRM_MODE_CONTENT_PROTECTION_OFF 0 +#define DRM_MODE_CONTENT_PROTECTION_DESIRED 1 +#define DRM_MODE_CONTENT_PROTECTION_ENABLED 2 struct drm_mode_modeinfo { __u32 clock;
This patch adds a new optional connector property to allow userspace to enable protection over the content it is displaying. This will typically be implemented by the driver using HDCP. The property is a tri-state with the following values: - OFF: Self explanatory, no content protection - DESIRED: Userspace requests that the driver enable protection - ENABLED: Once the driver has authenticated the link, it sets this value The driver is responsible for downgrading ENABLED to DESIRED if the link becomes unprotected. The driver should also maintain the desiredness of protection across hotplug/dpms/suspend. If this looks familiar, I posted [1] this 3 years ago. We have been using this in ChromeOS across exynos, mediatek, and rockchip over that time. Signed-off-by: Sean Paul <seanpaul@chromium.org> [1] https://lists.freedesktop.org/archives/dri-devel/2014-December/073336.html --- drivers/gpu/drm/drm_atomic.c | 8 ++++++++ drivers/gpu/drm/drm_connector.c | 43 +++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_sysfs.c | 29 +++++++++++++++++++++++++++ include/drm/drm_connector.h | 16 +++++++++++++++ include/uapi/drm/drm_mode.h | 4 ++++ 5 files changed, 100 insertions(+)