Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed
One Thousand Gnomes 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
Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed
On Tue, 29 Nov 2016 14:03:31 + David Howells 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
Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed
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
-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
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
Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed
David Howells 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
Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed
On 11/28/2016 06:11 PM, David Howells wrote:
Corey Minyard 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
Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed
Corey Minyard 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
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 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
Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed
Dominik Brodowski wrote: > For most cases, request_firmware() is being used -- for some rare cases, > however, this alternative interface is provided for. It should be pretty > safe nowadays to make pccard_cis_attr read-only (and who uses PCMCIA > nowadays anyway?). Have a look at the top patch here: http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=efi-lock-down David
Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed
On Thu, Nov 24, 2016 at 05:34:45PM +, David Howells wrote: > Dominik Brodowski wrote: > > > > > - Abort in drivers/pcmcia/cistpl.c::pccard_store_cis() or remove > > > > write access to the "cis" file in > > > > drivers/pcmcia/cistpl.c::pccard_cis_attr > > > > > > What is that doing? Allowing the device to be reconfigured? > > > > Exactly. It is a different interface for updating the firmware -- which > > includes ioports etc. In theory, any access should be limited to areas which > > are registered to the bridge devices. But you never know... > > Ah, I see. Should this be using request_firmware()? For most cases, request_firmware() is being used -- for some rare cases, however, this alternative interface is provided for. It should be pretty safe nowadays to make pccard_cis_attr read-only (and who uses PCMCIA nowadays anyway?). Best, Dominik
Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed
Dominik Brodowski wrote: > > > - Abort in drivers/pcmcia/cistpl.c::pccard_store_cis() or remove > > > write access to the "cis" file in > > > drivers/pcmcia/cistpl.c::pccard_cis_attr > > > > What is that doing? Allowing the device to be reconfigured? > > Exactly. It is a different interface for updating the firmware -- which > includes ioports etc. In theory, any access should be limited to areas which > are registered to the bridge devices. But you never know... Ah, I see. Should this be using request_firmware()? David
Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed
On Wed, Nov 23, 2016 at 12:58:26PM +, David Howells wrote: > Dominik Brodowski wrote: > > > You might also need to disable CIS overrides and CIS firmware loading for > > PCMCIA drivers, I presume. That needs two changes: > > > > - Abort in drivers/pcmcia/ds.c::pcmcia_load_firmware() or disable > > the CONFIG_PCMCIA_LOAD_CIS config option permanently. > > This really ought to be handled through signature checking in > request_firmware(). > > > - Abort in drivers/pcmcia/cistpl.c::pccard_store_cis() or remove > > write access to the "cis" file in > > drivers/pcmcia/cistpl.c::pccard_cis_attr > > What is that doing? Allowing the device to be reconfigured? Exactly. It is a different interface for updating the firmware -- which includes ioports etc. In theory, any access should be limited to areas which are registered to the bridge devices. But you never know... Best Dominik
Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed
Dominik Brodowski wrote: > You might also need to disable CIS overrides and CIS firmware loading for > PCMCIA drivers, I presume. That needs two changes: > > - Abort in drivers/pcmcia/ds.c::pcmcia_load_firmware() or disable > the CONFIG_PCMCIA_LOAD_CIS config option permanently. This really ought to be handled through signature checking in request_firmware(). > - Abort in drivers/pcmcia/cistpl.c::pccard_store_cis() or remove > write access to the "cis" file in > drivers/pcmcia/cistpl.c::pccard_cis_attr What is that doing? Allowing the device to be reconfigured? David
Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed
On Mon, Nov 21, 2016 at 11:10:52PM +, David Howells wrote: > One Thousand Gnomes 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 > --- > commit 8613a9655dad98c3358d82a9c4310cebdcb852ae > Author: David Howells > Date: Mon Nov 21 22:43:27 2016 + > > Lock down drivers that can have io ports, io mem, irqs and dma changed > > Lock down drivers that can have io ports, io mem, irqs and dma channels > changed so that they can't be used to cause hardware to access the kernel > image. > > Notes: > > (1) module_isa_driver() gets an extra parameter that, if true, will cause > the module load to be rejected if the kernel is locked down. > > (2) module_driver() calls module_lockdown_check() to ask if the module > load should be rejected if the kernel is locked down. This is a > macro > that should be #undef'd and then redefined right before > module_driver() is called. > > (3) module_pci_driver() is a wrapper around module_driver(), so the same > macro is used as in (2). > > (4) A number of drivers use parport 'ports' - so I haven't touched those. You might also need to disable CIS overrides and CIS firmware loading for PCMCIA drivers, I presume. That needs two changes: - Abort in drivers/pcmcia/ds.c::pcmcia_load_firmware() or disable the CONFIG_PCMCIA_LOAD_CIS config option permanently. - Abort in drivers/pcmcia/cistpl.c::pccard_store_cis() or remove write access to the "cis" file in drivers/pcmcia/cistpl.c::pccard_cis_attr Best, Dominik
