Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1531751 > unrolled thread

Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs  and dma changed Corey Minyard <minyard@acm.org> - 2016-11-28 23:40 +0100
    Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed David Howells <dhowells@redhat.com> - 2016-11-29 01:20 +0100
      Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs  and dma changed Corey Minyard <minyard@acm.org> - 2016-11-29 01:30 +0100
        Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed David Howells <dhowells@redhat.com> - 2016-11-29 15:10 +0100
          Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs  and dma changed Corey Minyard <minyard@acm.org> - 2016-11-29 15:40 +0100
          Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs  and dma changed One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-11-30 15:50 +0100
            Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed David Howells <dhowells@redhat.com> - 2016-11-30 17:30 +0100
      Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed David Howells <dhowells@redhat.com> - 2016-11-29 11:50 +0100

#1531751 — Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed

On 11/21/2016 05:10 PM, David Howells wrote:
> One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
>
>> You need to filter or lock down kernel module options because a lot of
>> modules let you set the I/O port or similar (eg mmio) which means you can
>> hack the entire machine with say the 8250 driver just by using it with an
>> mmio of the right location to patch the secure state to zero just by
>> getting the ability to write to the modules conf file.
> Is the attached patch the right sort of idea?  [Note that I haven't actually
> compiled most of these drivers to check my changes yet.]
>
> David
> ---

snip

> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index a112c0146012..7fb9c299a183 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -3725,6 +3725,12 @@ static int init_ipmi_si(void)
>   	struct smi_info *e;
>   	enum ipmi_addr_src type = SI_INVALID;
>   
> +	if ((num_addrs || num_ports || num_irqs) &&
> +	    kernel_is_locked_down()) {
> +		pr_err(PFX "Kernel is locked down\n");
> +		return -EPERM;
> +	}
> +
>   	if (initialized)
>   		return 0;
>   	initialized = 1;

This would prevent any IPMI interface from working if any address was given
on the kernel command line. I'm not sure what the best policy is, but that
sounds like a possible DOS to me.

Can you put this check in hardcode_find_bmc()?  Thats the only place where
the hardcoded addresses are used, and a check there won't affect anything
else.

Also, the error message sounds a little vague to me.  If I was a sysadmin
and got this, I wouldn't be sure what was going on.  Maybe something like:
The kernel is locked down, but hard-coded device addresses were given on
the driver command line.  Ignoring these, but this is a possible 
security issue.

That's fairly wordy, but it gets the point across.  You could also move the
pr_err() into kernel_is_locked_down() and pass in the prefix, since there is
basically the same pr_err() after every check.

Thanks,

-corey

[toc] | [next] | [standalone]

#1531813 — Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed

Corey Minyard <minyard@acm.org> wrote:

> This would prevent any IPMI interface from working if any address was given
> on the kernel command line. I'm not sure what the best policy is, but that
> sounds like a possible DOS to me.

Okay, reasonable point.

> Can you put this check in hardcode_find_bmc()?  Thats the only place where
> the hardcoded addresses are used, and a check there won't affect anything
> else.

I could do that.  I presume you'd want hardcode_find_bmc() to return 1 in that
case without doing anything else.  Another possibility is to give a warning
and then clear ports[], addrs[] and irqs[].

> Also, the error message sounds a little vague to me.  If I was a sysadmin
> and got this, I wouldn't be sure what was going on.  Maybe something like:
> The kernel is locked down, but hard-coded device addresses were given on
> the driver command line.  Ignoring these, but this is a possible security
> issue.
>
> That's fairly wordy, but it gets the point across.  You could also move the
> pr_err() into kernel_is_locked_down() and pass in the prefix, since there is
> basically the same pr_err() after every check.

I don't think your suggested summary quite gets it right.  A lot of drivers,
sound drivers, for example, that aren't really critical can simply be
disabled - and some have to be disabled because there's no other way to
configure them.

It would have to be more like pr_err("Hard-coded device addresses, irqs and
dma channels are not permitted when the kernel is locked down."), possibly
with the addition of either "The driver has been disabled" or "These settings
have been ignored".

David

[toc] | [prev] | [next] | [standalone]

#1531816

On 11/28/2016 06:11 PM, David Howells wrote:
> Corey Minyard <minyard@acm.org> wrote:
>
>> This would prevent any IPMI interface from working if any address was given
>> on the kernel command line. I'm not sure what the best policy is, but that
>> sounds like a possible DOS to me.
> Okay, reasonable point.
>
>> Can you put this check in hardcode_find_bmc()?  Thats the only place where
>> the hardcoded addresses are used, and a check there won't affect anything
>> else.
> I could do that.  I presume you'd want hardcode_find_bmc() to return 1 in that
> case without doing anything else.  Another possibility is to give a warning
> and then clear ports[], addrs[] and irqs[].
>

Just returning -EPERM from that routine is fine, without doing anything
else.  You can basically just move your check to the top of that
routine.

>> Also, the error message sounds a little vague to me.  If I was a sysadmin
>> and got this, I wouldn't be sure what was going on.  Maybe something like:
>> The kernel is locked down, but hard-coded device addresses were given on
>> the driver command line.  Ignoring these, but this is a possible security
>> issue.
>>
>> That's fairly wordy, but it gets the point across.  You could also move the
>> pr_err() into kernel_is_locked_down() and pass in the prefix, since there is
>> basically the same pr_err() after every check.
> I don't think your suggested summary quite gets it right.  A lot of drivers,
> sound drivers, for example, that aren't really critical can simply be
> disabled - and some have to be disabled because there's no other way to
> configure them.

Yeah.  My main issue was that the sysadmin would see this and not
have any idea what was going on.

>
> It would have to be more like pr_err("Hard-coded device addresses, irqs and
> dma channels are not permitted when the kernel is locked down."), possibly
> with the addition of either "The driver has been disabled" or "These settings
> have been ignored".

That sounds better than what I had.

-corey

[toc] | [prev] | [next] | [standalone]

#1532323 — Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed

How about the attached?  Obviously it need extending to other drivers.

I thought that if I'm changing the module_param annotations anyway then it's
probably worth bunging in an extra parameter that notes what the parameter
modifies (ioport, iomem, etc.) for future reference, even if we don't store
it.

David
---
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 7fb9c299a183..157e96391eca 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -1375,39 +1375,39 @@ MODULE_PARM_DESC(type, "Defines the type of each interface, each"
 		 " interface separated by commas.  The types are 'kcs',"
 		 " 'smic', and 'bt'.  For example si_type=kcs,bt will set"
 		 " the first interface to kcs and the second to bt");
-module_param_array(addrs, ulong, &num_addrs, 0);
+module_param_hw_array(addrs, ulong, iomem, &num_addrs, 0);
 MODULE_PARM_DESC(addrs, "Sets the memory address of each interface, the"
 		 " addresses separated by commas.  Only use if an interface"
 		 " is in memory.  Otherwise, set it to zero or leave"
 		 " it blank.");
-module_param_array(ports, uint, &num_ports, 0);
+module_param_hw_array(ports, uint, ioport, &num_ports, 0);
 MODULE_PARM_DESC(ports, "Sets the port address of each interface, the"
 		 " addresses separated by commas.  Only use if an interface"
 		 " is a port.  Otherwise, set it to zero or leave"
 		 " it blank.");
-module_param_array(irqs, int, &num_irqs, 0);
+module_param_hw_array(irqs, int, irq, &num_irqs, 0);
 MODULE_PARM_DESC(irqs, "Sets the interrupt of each interface, the"
 		 " addresses separated by commas.  Only use if an interface"
 		 " has an interrupt.  Otherwise, set it to zero or leave"
 		 " it blank.");
-module_param_array(regspacings, int, &num_regspacings, 0);
+module_param_hw_array(regspacings, int, other, &num_regspacings, 0);
 MODULE_PARM_DESC(regspacings, "The number of bytes between the start address"
 		 " and each successive register used by the interface.  For"
 		 " instance, if the start address is 0xca2 and the spacing"
 		 " is 2, then the second address is at 0xca4.  Defaults"
 		 " to 1.");
-module_param_array(regsizes, int, &num_regsizes, 0);
+module_param_hw_array(regsizes, int, other, &num_regsizes, 0);
 MODULE_PARM_DESC(regsizes, "The size of the specific IPMI register in bytes."
 		 " This should generally be 1, 2, 4, or 8 for an 8-bit,"
 		 " 16-bit, 32-bit, or 64-bit register.  Use this if you"
 		 " the 8-bit IPMI register has to be read from a larger"
 		 " register.");
-module_param_array(regshifts, int, &num_regshifts, 0);
+module_param_hw_array(regshifts, int, other, &num_regshifts, 0);
 MODULE_PARM_DESC(regshifts, "The amount to shift the data read from the."
 		 " IPMI register, in bits.  For instance, if the data"
 		 " is read from a 32-bit word and the IPMI data is in"
 		 " bit 8-15, then the shift would be 8");
-module_param_array(slave_addrs, int, &num_slave_addrs, 0);
+module_param_hw_array(slave_addrs, int, other, &num_slave_addrs, 0);
 MODULE_PARM_DESC(slave_addrs, "Set the default IPMB slave address for"
 		 " the controller.  Normally this is 0x20, but can be"
 		 " overridden by this parm.  This is an array indexed"
@@ -3725,12 +3725,6 @@ static int init_ipmi_si(void)
 	struct smi_info *e;
 	enum ipmi_addr_src type = SI_INVALID;
 
-	if ((num_addrs || num_ports || num_irqs) &&
-	    kernel_is_locked_down()) {
-		pr_err(PFX "Kernel is locked down\n");
-		return -EPERM;
-	}
-
 	if (initialized)
 		return 0;
 	initialized = 1;
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 52666d90ca94..bdb884fba79a 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -60,9 +60,11 @@ struct kernel_param_ops {
  * Flags available for kernel_param
  *
  * UNSAFE - the parameter is dangerous and setting it will taint the kernel
+ * HWPARAM - Hardware param not permitted in lockdown mode
  */
 enum {
-	KERNEL_PARAM_FL_UNSAFE = (1 << 0)
+	KERNEL_PARAM_FL_UNSAFE	= (1 << 0),
+	KERNEL_PARAM_FL_HWPARAM	= (1 << 1),
 };
 
 struct kernel_param {
@@ -451,6 +453,62 @@ extern int param_set_bint(const char *val, const struct kernel_param *kp);
 			    perm, -1, 0);				\
 	__MODULE_PARM_TYPE(name, "array of " #type)
 
+enum hwparam_type {
+	hwparam_ioport,		/* Module parameter configures an I/O port */
+	hwparam_iomem,		/* Module parameter configures an I/O mem address */
+	hwparam_irq,		/* Module parameter configures an I/O port */
+	hwparam_dma,		/* Module parameter configures a DMA channel */
+	hwparam_dma_addr,	/* Module parameter configures a DMA buffer address */
+	hwparam_other,		/* Module parameter configures some other value */
+};
+
+/**
+ * module_param_hw - A parameter representing a hw parameters
+ * @name: a valid C identifier which is the parameter name.
+ * @type: the type of the parameter
+ * @hwtype: what the value represents (enum hwparam_type)
+ * @perm: visibility in sysfs.
+ *
+ * Usually it's a good idea to have variable names and user-exposed names the
+ * same, but that's harder if the variable must be non-static or is inside a
+ * structure.  This allows exposure under a different name.
+ */
+#define module_param_hw(name, type, hwtype, perm)			\
+	param_check_##type(name, &(name));				\
+	__module_param_call(MODULE_PARAM_PREFIX, name,			\
+			    &param_ops_##type, &name,			\
+			    perm, -1,					\
+			    KERNEL_PARAM_FL_HWPARAM | (hwparam_##hwtype & 0));	\
+	__MODULE_PARM_TYPE(name, #type)
+
+/**
+ * module_param_hw_array - A parameter representing an array of hw parameters
+ * @name: the name of the array variable
+ * @type: the type, as per module_param()
+ * @hwtype: what the value represents (enum hwparam_type)
+ * @nump: optional pointer filled in with the number written
+ * @perm: visibility in sysfs
+ *
+ * Input and output are as comma-separated values.  Commas inside values
+ * don't work properly (eg. an array of charp).
+ *
+ * ARRAY_SIZE(@name) is used to determine the number of elements in the
+ * array, so the definition must be visible.
+ */
+#define module_param_hw_array(name, type, hwtype, nump, perm)		\
+	param_check_##type(name, &(name)[0]);				\
+	static const struct kparam_array __param_arr_##name		\
+	= { .max = ARRAY_SIZE(name), .num = nump,			\
+	    .ops = &param_ops_##type,					\
+	    .elemsize = sizeof(name[0]), .elem = name };		\
+	__module_param_call(MODULE_PARAM_PREFIX, name,			\
+			    &param_array_ops,				\
+			    .arr = &__param_arr_##name,			\
+			    perm, -1,					\
+			    KERNEL_PARAM_FL_HWPARAM | (hwparam_##hwtype & 0));	\
+	__MODULE_PARM_TYPE(name, "array of " #type)
+
+
 extern const struct kernel_param_ops param_array_ops;
 
 extern const struct kernel_param_ops param_ops_string;
diff --git a/kernel/params.c b/kernel/params.c
index a6d6149c0fe6..2b68e6dad677 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -108,13 +108,20 @@ bool parameq(const char *a, const char *b)
 	return parameqn(a, b, strlen(a)+1);
 }
 
-static void param_check_unsafe(const struct kernel_param *kp)
+static bool param_check_unsafe(const struct kernel_param *kp,
+			       const char *doing)
 {
 	if (kp->flags & KERNEL_PARAM_FL_UNSAFE) {
 		pr_warn("Setting dangerous option %s - tainting kernel\n",
 			kp->name);
 		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
 	}
+
+	if (kp->flags & KERNEL_PARAM_FL_HWPARAM && kernel_is_locked_down()) {
+		pr_err("Command line-specified device addresses, irqs and dma channels are not permitted when the kernel is locked down (%s.%s)\n", doing, kp->name);
+		return false;
+	}
+	return true;
 }
 
 static int parse_one(char *param,
@@ -144,8 +151,10 @@ static int parse_one(char *param,
 			pr_debug("handling %s with %p\n", param,
 				params[i].ops->set);
 			kernel_param_lock(params[i].mod);
-			param_check_unsafe(&params[i]);
-			err = params[i].ops->set(val, &params[i]);
+			if (param_check_unsafe(&params[i], doing))
+				err = params[i].ops->set(val, &params[i]);
+			else
+				err = -EPERM;
 			kernel_param_unlock(params[i].mod);
 			return err;
 		}
@@ -620,8 +629,10 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
 		return -EPERM;
 
 	kernel_param_lock(mk->mod);
-	param_check_unsafe(attribute->param);
-	err = attribute->param->ops->set(buf, attribute->param);
+	if (param_check_unsafe(attribute->param, mk->mod->name))
+		err = attribute->param->ops->set(buf, attribute->param);
+	else
+		err = -EPERM;
 	kernel_param_unlock(mk->mod);
 	if (!err)
 		return len;

[toc] | [prev] | [next] | [standalone]

#1532344

On 11/29/2016 08:03 AM, David Howells wrote:
> How about the attached?  Obviously it need extending to other drivers.

This is great, I like it a lot better.

Reviewed-by: Corey Minyard <cminyard@mvista.com>

-corey

> I thought that if I'm changing the module_param annotations anyway then it's
> probably worth bunging in an extra parameter that notes what the parameter
> modifies (ioport, iomem, etc.) for future reference, even if we don't store
> it.
>
> David
> ---
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 7fb9c299a183..157e96391eca 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -1375,39 +1375,39 @@ MODULE_PARM_DESC(type, "Defines the type of each interface, each"
>   		 " interface separated by commas.  The types are 'kcs',"
>   		 " 'smic', and 'bt'.  For example si_type=kcs,bt will set"
>   		 " the first interface to kcs and the second to bt");
> -module_param_array(addrs, ulong, &num_addrs, 0);
> +module_param_hw_array(addrs, ulong, iomem, &num_addrs, 0);
>   MODULE_PARM_DESC(addrs, "Sets the memory address of each interface, the"
>   		 " addresses separated by commas.  Only use if an interface"
>   		 " is in memory.  Otherwise, set it to zero or leave"
>   		 " it blank.");
> -module_param_array(ports, uint, &num_ports, 0);
> +module_param_hw_array(ports, uint, ioport, &num_ports, 0);
>   MODULE_PARM_DESC(ports, "Sets the port address of each interface, the"
>   		 " addresses separated by commas.  Only use if an interface"
>   		 " is a port.  Otherwise, set it to zero or leave"
>   		 " it blank.");
> -module_param_array(irqs, int, &num_irqs, 0);
> +module_param_hw_array(irqs, int, irq, &num_irqs, 0);
>   MODULE_PARM_DESC(irqs, "Sets the interrupt of each interface, the"
>   		 " addresses separated by commas.  Only use if an interface"
>   		 " has an interrupt.  Otherwise, set it to zero or leave"
>   		 " it blank.");
> -module_param_array(regspacings, int, &num_regspacings, 0);
> +module_param_hw_array(regspacings, int, other, &num_regspacings, 0);
>   MODULE_PARM_DESC(regspacings, "The number of bytes between the start address"
>   		 " and each successive register used by the interface.  For"
>   		 " instance, if the start address is 0xca2 and the spacing"
>   		 " is 2, then the second address is at 0xca4.  Defaults"
>   		 " to 1.");
> -module_param_array(regsizes, int, &num_regsizes, 0);
> +module_param_hw_array(regsizes, int, other, &num_regsizes, 0);
>   MODULE_PARM_DESC(regsizes, "The size of the specific IPMI register in bytes."
>   		 " This should generally be 1, 2, 4, or 8 for an 8-bit,"
>   		 " 16-bit, 32-bit, or 64-bit register.  Use this if you"
>   		 " the 8-bit IPMI register has to be read from a larger"
>   		 " register.");
> -module_param_array(regshifts, int, &num_regshifts, 0);
> +module_param_hw_array(regshifts, int, other, &num_regshifts, 0);
>   MODULE_PARM_DESC(regshifts, "The amount to shift the data read from the."
>   		 " IPMI register, in bits.  For instance, if the data"
>   		 " is read from a 32-bit word and the IPMI data is in"
>   		 " bit 8-15, then the shift would be 8");
> -module_param_array(slave_addrs, int, &num_slave_addrs, 0);
> +module_param_hw_array(slave_addrs, int, other, &num_slave_addrs, 0);
>   MODULE_PARM_DESC(slave_addrs, "Set the default IPMB slave address for"
>   		 " the controller.  Normally this is 0x20, but can be"
>   		 " overridden by this parm.  This is an array indexed"
> @@ -3725,12 +3725,6 @@ static int init_ipmi_si(void)
>   	struct smi_info *e;
>   	enum ipmi_addr_src type = SI_INVALID;
>   
> -	if ((num_addrs || num_ports || num_irqs) &&
> -	    kernel_is_locked_down()) {
> -		pr_err(PFX "Kernel is locked down\n");
> -		return -EPERM;
> -	}
> -
>   	if (initialized)
>   		return 0;
>   	initialized = 1;
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 52666d90ca94..bdb884fba79a 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -60,9 +60,11 @@ struct kernel_param_ops {
>    * Flags available for kernel_param
>    *
>    * UNSAFE - the parameter is dangerous and setting it will taint the kernel
> + * HWPARAM - Hardware param not permitted in lockdown mode
>    */
>   enum {
> -	KERNEL_PARAM_FL_UNSAFE = (1 << 0)
> +	KERNEL_PARAM_FL_UNSAFE	= (1 << 0),
> +	KERNEL_PARAM_FL_HWPARAM	= (1 << 1),
>   };
>   
>   struct kernel_param {
> @@ -451,6 +453,62 @@ extern int param_set_bint(const char *val, const struct kernel_param *kp);
>   			    perm, -1, 0);				\
>   	__MODULE_PARM_TYPE(name, "array of " #type)
>   
> +enum hwparam_type {
> +	hwparam_ioport,		/* Module parameter configures an I/O port */
> +	hwparam_iomem,		/* Module parameter configures an I/O mem address */
> +	hwparam_irq,		/* Module parameter configures an I/O port */
> +	hwparam_dma,		/* Module parameter configures a DMA channel */
> +	hwparam_dma_addr,	/* Module parameter configures a DMA buffer address */
> +	hwparam_other,		/* Module parameter configures some other value */
> +};
> +
> +/**
> + * module_param_hw - A parameter representing a hw parameters
> + * @name: a valid C identifier which is the parameter name.
> + * @type: the type of the parameter
> + * @hwtype: what the value represents (enum hwparam_type)
> + * @perm: visibility in sysfs.
> + *
> + * Usually it's a good idea to have variable names and user-exposed names the
> + * same, but that's harder if the variable must be non-static or is inside a
> + * structure.  This allows exposure under a different name.
> + */
> +#define module_param_hw(name, type, hwtype, perm)			\
> +	param_check_##type(name, &(name));				\
> +	__module_param_call(MODULE_PARAM_PREFIX, name,			\
> +			    &param_ops_##type, &name,			\
> +			    perm, -1,					\
> +			    KERNEL_PARAM_FL_HWPARAM | (hwparam_##hwtype & 0));	\
> +	__MODULE_PARM_TYPE(name, #type)
> +
> +/**
> + * module_param_hw_array - A parameter representing an array of hw parameters
> + * @name: the name of the array variable
> + * @type: the type, as per module_param()
> + * @hwtype: what the value represents (enum hwparam_type)
> + * @nump: optional pointer filled in with the number written
> + * @perm: visibility in sysfs
> + *
> + * Input and output are as comma-separated values.  Commas inside values
> + * don't work properly (eg. an array of charp).
> + *
> + * ARRAY_SIZE(@name) is used to determine the number of elements in the
> + * array, so the definition must be visible.
> + */
> +#define module_param_hw_array(name, type, hwtype, nump, perm)		\
> +	param_check_##type(name, &(name)[0]);				\
> +	static const struct kparam_array __param_arr_##name		\
> +	= { .max = ARRAY_SIZE(name), .num = nump,			\
> +	    .ops = &param_ops_##type,					\
> +	    .elemsize = sizeof(name[0]), .elem = name };		\
> +	__module_param_call(MODULE_PARAM_PREFIX, name,			\
> +			    &param_array_ops,				\
> +			    .arr = &__param_arr_##name,			\
> +			    perm, -1,					\
> +			    KERNEL_PARAM_FL_HWPARAM | (hwparam_##hwtype & 0));	\
> +	__MODULE_PARM_TYPE(name, "array of " #type)
> +
> +
>   extern const struct kernel_param_ops param_array_ops;
>   
>   extern const struct kernel_param_ops param_ops_string;
> diff --git a/kernel/params.c b/kernel/params.c
> index a6d6149c0fe6..2b68e6dad677 100644
> --- a/kernel/params.c
> +++ b/kernel/params.c
> @@ -108,13 +108,20 @@ bool parameq(const char *a, const char *b)
>   	return parameqn(a, b, strlen(a)+1);
>   }
>   
> -static void param_check_unsafe(const struct kernel_param *kp)
> +static bool param_check_unsafe(const struct kernel_param *kp,
> +			       const char *doing)
>   {
>   	if (kp->flags & KERNEL_PARAM_FL_UNSAFE) {
>   		pr_warn("Setting dangerous option %s - tainting kernel\n",
>   			kp->name);
>   		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
>   	}
> +
> +	if (kp->flags & KERNEL_PARAM_FL_HWPARAM && kernel_is_locked_down()) {
> +		pr_err("Command line-specified device addresses, irqs and dma channels are not permitted when the kernel is locked down (%s.%s)\n", doing, kp->name);
> +		return false;
> +	}
> +	return true;
>   }
>   
>   static int parse_one(char *param,
> @@ -144,8 +151,10 @@ static int parse_one(char *param,
>   			pr_debug("handling %s with %p\n", param,
>   				params[i].ops->set);
>   			kernel_param_lock(params[i].mod);
> -			param_check_unsafe(&params[i]);
> -			err = params[i].ops->set(val, &params[i]);
> +			if (param_check_unsafe(&params[i], doing))
> +				err = params[i].ops->set(val, &params[i]);
> +			else
> +				err = -EPERM;
>   			kernel_param_unlock(params[i].mod);
>   			return err;
>   		}
> @@ -620,8 +629,10 @@ static ssize_t param_attr_store(struct module_attribute *mattr,
>   		return -EPERM;
>   
>   	kernel_param_lock(mk->mod);
> -	param_check_unsafe(attribute->param);
> -	err = attribute->param->ops->set(buf, attribute->param);
> +	if (param_check_unsafe(attribute->param, mk->mod->name))
> +		err = attribute->param->ops->set(buf, attribute->param);
> +	else
> +		err = -EPERM;
>   	kernel_param_unlock(mk->mod);
>   	if (!err)
>   		return len;

[toc] | [prev] | [next] | [standalone]

#1533355

On Tue, 29 Nov 2016 14:03:31 +0000
David Howells <dhowells@redhat.com> wrote:

> How about the attached?  Obviously it need extending to other drivers.
> 
> I thought that if I'm changing the module_param annotations anyway then it's
> probably worth bunging in an extra parameter that notes what the parameter
> modifies (ioport, iomem, etc.) for future reference, even if we don't store
> it.


With a security hat on the security best practice and long standing
accepted rule is that you whitelist rather than blacklist, so there ought
to be a 

module_param_safe_array()
etc

to mark parameters that are safe, not the reverse.


That debate aside I think the patch is exactly what is needed for this,
and is probably useful for more general hardening as well.

Alan

[toc] | [prev] | [next] | [standalone]

#1533417 — Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed

One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:

> > I thought that if I'm changing the module_param annotations anyway then it's
> > probably worth bunging in an extra parameter that notes what the parameter
> > modifies (ioport, iomem, etc.) for future reference, even if we don't store
> > it.
> 
> With a security hat on the security best practice and long standing
> accepted rule is that you whitelist rather than blacklist, so there ought
> to be a 
> 
> module_param_safe_array()
> etc
> 
> to mark parameters that are safe, not the reverse.

Whilst that may be true, it's a lot more work.  Mind you, that said, you can
take the annotations I've made and script the inverse.

> That debate aside I think the patch is exactly what is needed for this,
> and is probably useful for more general hardening as well.

Okay, I've done a preliminary patchset, labelling all the parameters that
appear to be associated with hardware details and pushed them here:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=efi-lock-down

The patch that modifies the module parameter header is labelled:

	Lock down module params that specify hardware parameters (eg. ioport)

and all the driver dir lockdown patches follow that.

I still need to do a bit of commenting in that patch and add various
maintainer cc's in the other patches.

David

[toc] | [prev] | [next] | [standalone]

#1532121 — Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed

David Howells <dhowells@redhat.com> wrote:

> It would have to be more like pr_err("Hard-coded device addresses, irqs and
> dma channels are not permitted when the kernel is locked down."), possibly
> with the addition of either "The driver has been disabled" or "These settings
> have been ignored".

That should be "Command line-specified" rather than "Hard-coded".  The latter
are actually okay.

A better way to do this would probably be to annotate the module parameter
declarations and have the module_param() invoker reject the locked-down
parameters.  I'm not sure how easy that would be to do, though.

David

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web