| Message ID | 6973.1480428211@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, \ > + ¶m_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 = ¶m_ops_##type, \ > + .elemsize = sizeof(name[0]), .elem = name }; \ > + __module_param_call(MODULE_PARAM_PREFIX, name, \ > + ¶m_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(¶ms[i]); > - err = params[i].ops->set(val, ¶ms[i]); > + if (param_check_unsafe(¶ms[i], doing)) > + err = params[i].ops->set(val, ¶ms[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; -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, \ + ¶m_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 = ¶m_ops_##type, \ + .elemsize = sizeof(name[0]), .elem = name }; \ + __module_param_call(MODULE_PARAM_PREFIX, name, \ + ¶m_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(¶ms[i]); - err = params[i].ops->set(val, ¶ms[i]); + if (param_check_unsafe(¶ms[i], doing)) + err = params[i].ops->set(val, ¶ms[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;