RE: [Intel-gfx] [PATCH 03/11] drm/i915: Allow the sysadmin to override security mitigations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Chris
> Wilson
> Sent: Sunday, January 10, 2021 7:04 AM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: stable@xxxxxxxxxxxxxxx; Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Subject: [Intel-gfx] [PATCH 03/11] drm/i915: Allow the sysadmin to override
> security mitigations
> 
> The clear-residuals mitigation is a relatively heavy hammer and under some
> circumstances the user may wish to forgo the context isolation in order to
> meet some performance requirement. Introduce a generic module parameter
> to allow selectively enabling/disabling different mitigations.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/1858
> Fixes: 47f8253d2b89 ("drm/i915/gen7: Clear all EU/L3 residual contexts")
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Jon Bloomfield <jon.bloomfield@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # v5.7
> ---
>  drivers/gpu/drm/i915/Makefile                 |   1 +
>  .../gpu/drm/i915/gt/intel_ring_submission.c   |   4 +-
>  drivers/gpu/drm/i915/i915_mitigations.c       | 148 ++++++++++++++++++
>  drivers/gpu/drm/i915/i915_mitigations.h       |  13 ++
>  4 files changed, 165 insertions(+), 1 deletion(-)  create mode 100644
> drivers/gpu/drm/i915/i915_mitigations.c
>  create mode 100644 drivers/gpu/drm/i915/i915_mitigations.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 4074d8cb0d6e..48f82c354611 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -38,6 +38,7 @@ i915-y += i915_drv.o \
>  	  i915_config.o \
>  	  i915_irq.o \
>  	  i915_getparam.o \
> +	  i915_mitigations.o \
>  	  i915_params.o \
>  	  i915_pci.o \
>  	  i915_scatterlist.o \
> diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> index 724d56c9583d..657afd8ebc14 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> @@ -32,6 +32,7 @@
>  #include "gen6_ppgtt.h"
>  #include "gen7_renderclear.h"
>  #include "i915_drv.h"
> +#include "i915_mitigations.h"
>  #include "intel_breadcrumbs.h"
>  #include "intel_context.h"
>  #include "intel_gt.h"
> @@ -918,7 +919,8 @@ static int switch_context(struct i915_request *rq)
>  	GEM_BUG_ON(HAS_EXECLISTS(engine->i915));
> 
>  	if (engine->wa_ctx.vma && ce != engine->kernel_context) {
> -		if (engine->wa_ctx.vma->private != ce) {
> +		if (engine->wa_ctx.vma->private != ce &&
> +		    i915_mitigate_clear_residuals()) {
>  			ret = clear_residuals(rq);
>  			if (ret)
>  				return ret;
> diff --git a/drivers/gpu/drm/i915/i915_mitigations.c
> b/drivers/gpu/drm/i915/i915_mitigations.c
> new file mode 100644
> index 000000000000..8d5637cfa734
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_mitigations.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2021 Intel Corporation
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/moduleparam.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +
> +#include "i915_drv.h"
> +#include "i915_mitigations.h"
> +
> +static unsigned long mitigations = ~0UL;
> +
> +enum {
> +	CLEAR_RESIDUALS = 0,
> +};
> +
> +static const char * const names[] = {
> +	[CLEAR_RESIDUALS] = "residuals",
> +};
> +
> +bool i915_mitigate_clear_residuals(void)
> +{
> +	return READ_ONCE(mitigations) & BIT(CLEAR_RESIDUALS); }
> +
> +static int mitigations_set(const char *val, const struct kernel_param
> +*kp) {
> +	unsigned long new = ~0UL;
> +	char *str, *sep, *tok;
> +	bool first = true;
> +	int err = 0;
> +
> +	BUILD_BUG_ON(ARRAY_SIZE(names) >=
> BITS_PER_TYPE(mitigations));
> +
> +	str = kstrdup(val, GFP_KERNEL);
> +	if (!str)
> +		return -ENOMEM;
> +
> +	for (sep = str; (tok = strsep(&sep, ","));) {
> +		bool enable = true;
> +		int i;
> +
> +		/* Be tolerant of leading/trailing whitespace */
> +		tok = strim(tok);
> +
> +		if (first) {
> +			first = false;
> +
> +			if (!strcmp(tok, "auto")) {
> +				new = ~0UL;
> +				continue;
> +			}
> +
> +			new = 0;
> +			if (!strcmp(tok, "off"))
> +				continue;
> +		}
> +
> +		if (*tok == '!') {
> +			enable = !enable;
> +			tok++;
> +		}
> +
> +		if (!strncmp(tok, "no", 2)) {
> +			enable = !enable;
> +			tok += 2;
> +		}
> +
> +		if (*tok == '\0')
> +			continue;
> +
> +		for (i = 0; i < ARRAY_SIZE(names); i++) {
> +			if (!strcmp(tok, names[i])) {
> +				if (enable)
> +					new |= BIT(i);
> +				else
> +					new &= ~BIT(i);
> +				break;
> +			}
> +		}
> +		if (i == ARRAY_SIZE(names)) {
> +			pr_err("Bad %s.mitigations=%s, '%s' is unknown\n",
> +			       DRIVER_NAME, val, tok);
> +			err = -EINVAL;
> +			break;
> +		}
> +	}
> +	kfree(str);
> +	if (err)
> +		return err;
> +
> +	WRITE_ONCE(mitigations, new);
> +	return 0;
> +}
> +
> +static int mitigations_get(char *buffer, const struct kernel_param *kp)
> +{
> +	unsigned long local = READ_ONCE(mitigations);
> +	int count, i;
> +	bool enable;
> +
> +	if (!local)
> +		return scnprintf(buffer, PAGE_SIZE, "%s\n", "off");
> +
> +	if (local & BIT(BITS_PER_LONG - 1)) {
> +		count = scnprintf(buffer, PAGE_SIZE, "%s,", "auto");
> +		enable = false;
> +	} else {
> +		enable = true;
> +		count = 0;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(names); i++) {
> +		if ((local & BIT(i)) != enable)
> +			continue;
> +
> +		count += scnprintf(buffer + count, PAGE_SIZE - count,
> +				   "%s%s,", enable ? "" : "!", names[i]);
> +	}
> +
> +	buffer[count - 1] = '\n';
> +	return count;
> +}
> +
> +static const struct kernel_param_ops ops = {
> +	.set = mitigations_set,
> +	.get = mitigations_get,
> +};
> +
> +module_param_cb_unsafe(mitigations, &ops, NULL, 0600);
> +MODULE_PARM_DESC(mitigations, "Selectively enable security mitigations
> +for all Intel® GPUs in the system.\n"
> +"\n"
> +"  auto -- enables all mitigations required for the platform [default]\n"
> +"  off  -- disables all mitigations\n"
> +"\n"
> +"Individual mitigations can be enabled by passing a comma-separated
> string,\n"
> +"e.g. mitigations=residuals to enable only clearing residuals or\n"
> +"mitigations=auto,noresiduals to disable only the clear residual
> mitigation.\n"
> +"Either '!' or 'no' may be used to switch from enabling the mitigation to\n"
> +"disabling it.\n"
> +"\n"
> +"Active mitigations for Ivybridge, Baytrail, Haswell:\n"
> +"  residuals -- clear all thread-local registers between contexts"
> +);
> diff --git a/drivers/gpu/drm/i915/i915_mitigations.h
> b/drivers/gpu/drm/i915/i915_mitigations.h
> new file mode 100644
> index 000000000000..1359d8135287
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_mitigations.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2021 Intel Corporation
> + */
> +
> +#ifndef __I915_MITIGATIONS_H__
> +#define __I915_MITIGATIONS_H__
> +
> +#include <linux/types.h>
> +
> +bool i915_mitigate_clear_residuals(void);
> +
> +#endif /* __I915_MITIGATIONS_H__ */

Although this seems like ideal solution - giving users option to choose *potential* performance over security or vice-versa -  However, I would have expected that this patch adds a DRM warning to inform users of the consequences of their action, whenever module parameter is used to disable any kind of mitigations. Well, that is my own perspective, not as a legal expert.

Thanks,
~Akeem




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux