| Message ID | 20190830154549.vss6h5tlrl6d5r5y@decadent.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] staging: comedi: Restrict COMEDI_DEVCONFIG when the kernel is locked down | expand |
On 30/08/2019 16:45, Ben Hutchings wrote: > The COMEDI_DEVCONFIG ioctl can be used to configure I/O addresses and > other hardware settings for non plug-and-play devices such as ISA > cards. This should be disabled to preserve the kernel's integrity > when it is locked down. I haven't boned up on the lockdown mechanism yet, but just FYI, this is only possible if the "comedi_num_legacy_minors" module parameter is non-zero (which it isn't by default). > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > Cc: Matthew Garrett <mjg59@google.com> > Cc: David Howells <dhowells@redhat.com> > Cc: Ian Abbott <abbotti@mev.co.uk> > Cc: H Hartley Sweeten <hsweeten@visionengravers.com> > --- > drivers/staging/comedi/comedi_fops.c | 6 ++++++ > include/linux/security.h | 1 + > security/lockdown/lockdown.c | 1 + > 3 files changed, 8 insertions(+) > > diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c > index f6d1287c7b83..fdf030e53035 100644 > --- a/drivers/staging/comedi/comedi_fops.c > +++ b/drivers/staging/comedi/comedi_fops.c > @@ -27,6 +27,7 @@ > > #include <linux/io.h> > #include <linux/uaccess.h> > +#include <linux/security.h> > > #include "comedi_internal.h" > > @@ -813,11 +814,16 @@ static int do_devconfig_ioctl(struct comedi_device *dev, > struct comedi_devconfig __user *arg) > { > struct comedi_devconfig it; > + int ret; > > lockdep_assert_held(&dev->mutex); > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > > + ret = security_locked_down(LOCKDOWN_COMEDI_DEVCONFIG); > + if (ret) > + return ret; > + You might consider moving that check to be done after the following 'if (!arg)' block, since that should be safe. (It detaches an already configured device from the comedi core.) > if (!arg) { > if (is_device_busy(dev)) > return -EBUSY; > diff --git a/include/linux/security.h b/include/linux/security.h > index 429f9f03372b..b16365dccfc5 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -113,6 +113,7 @@ enum lockdown_reason { > LOCKDOWN_ACPI_TABLES, > LOCKDOWN_PCMCIA_CIS, > LOCKDOWN_TIOCSSERIAL, > + LOCKDOWN_COMEDI_DEVCONFIG, > LOCKDOWN_MODULE_PARAMETERS, > LOCKDOWN_MMIOTRACE, > LOCKDOWN_DEBUGFS, > diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c > index 0068cec77c05..971bb99b9051 100644 > --- a/security/lockdown/lockdown.c > +++ b/security/lockdown/lockdown.c > @@ -28,6 +28,7 @@ static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = { > [LOCKDOWN_ACPI_TABLES] = "modifying ACPI tables", > [LOCKDOWN_PCMCIA_CIS] = "direct PCMCIA CIS storage", > [LOCKDOWN_TIOCSSERIAL] = "reconfiguration of serial port IO", > + [LOCKDOWN_COMEDI_DEVCONFIG] = "reconfiguration of Comedi legacy device", > [LOCKDOWN_MODULE_PARAMETERS] = "unsafe module parameters", > [LOCKDOWN_MMIOTRACE] = "unsafe mmio", > [LOCKDOWN_DEBUGFS] = "debugfs access", >
On Fri, 2019-08-30 at 18:35 +0100, Ian Abbott wrote: > On 30/08/2019 16:45, Ben Hutchings wrote: > > The COMEDI_DEVCONFIG ioctl can be used to configure I/O addresses and > > other hardware settings for non plug-and-play devices such as ISA > > cards. This should be disabled to preserve the kernel's integrity > > when it is locked down. > > I haven't boned up on the lockdown mechanism yet, but just FYI, this is > only possible if the "comedi_num_legacy_minors" module parameter is > non-zero (which it isn't by default). So do you think would it make more sense to set the HWPARAM flag on that module parameter? That should have the same effect although it doesn't seem to quite fit the intent of that flag. > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > > Cc: Matthew Garrett <mjg59@google.com> > > Cc: David Howells <dhowells@redhat.com> > > Cc: Ian Abbott <abbotti@mev.co.uk> > > Cc: H Hartley Sweeten <hsweeten@visionengravers.com> > > --- > > drivers/staging/comedi/comedi_fops.c | 6 ++++++ > > include/linux/security.h | 1 + > > security/lockdown/lockdown.c | 1 + > > 3 files changed, 8 insertions(+) > > > > diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c > > index f6d1287c7b83..fdf030e53035 100644 > > --- a/drivers/staging/comedi/comedi_fops.c > > +++ b/drivers/staging/comedi/comedi_fops.c > > @@ -27,6 +27,7 @@ > > > > #include <linux/io.h> > > #include <linux/uaccess.h> > > +#include <linux/security.h> > > > > #include "comedi_internal.h" > > > > @@ -813,11 +814,16 @@ static int do_devconfig_ioctl(struct comedi_device *dev, > > struct comedi_devconfig __user *arg) > > { > > struct comedi_devconfig it; > > + int ret; > > > > lockdep_assert_held(&dev->mutex); > > if (!capable(CAP_SYS_ADMIN)) > > return -EPERM; > > > > + ret = security_locked_down(LOCKDOWN_COMEDI_DEVCONFIG); > > + if (ret) > > + return ret; > > + > > You might consider moving that check to be done after the following 'if > (!arg)' block, since that should be safe. (It detaches an already > configured device from the comedi core.) [...] How would it have been configured, though? Ben.
On 31/08/2019 10:50, Ben Hutchings wrote: > On Fri, 2019-08-30 at 18:35 +0100, Ian Abbott wrote: >> On 30/08/2019 16:45, Ben Hutchings wrote: >>> The COMEDI_DEVCONFIG ioctl can be used to configure I/O addresses and >>> other hardware settings for non plug-and-play devices such as ISA >>> cards. This should be disabled to preserve the kernel's integrity >>> when it is locked down. >> >> I haven't boned up on the lockdown mechanism yet, but just FYI, this is >> only possible if the "comedi_num_legacy_minors" module parameter is >> non-zero (which it isn't by default). > > So do you think would it make more sense to set the HWPARAM flag on > that module parameter? That should have the same effect although it > doesn't seem to quite fit the intent of that flag. HWPARAM would prohibit the creation of a few special comedi devices such as those created by the "comedi_test" and "comedi_bond" drivers. (Although one dummy device does get created by the "comedi_test" module when it is loaded, and I don't know if anyone actually uses the "comedi_bond" driver!) But then again, the changes to COMEDI_DEVCONFIG also prohibits the creation of those special devices, so I don't suppose it matters either way. > >>> Signed-off-by: Ben Hutchings <ben@decadent.org.uk> >>> Cc: Matthew Garrett <mjg59@google.com> >>> Cc: David Howells <dhowells@redhat.com> >>> Cc: Ian Abbott <abbotti@mev.co.uk> >>> Cc: H Hartley Sweeten <hsweeten@visionengravers.com> >>> --- >>> drivers/staging/comedi/comedi_fops.c | 6 ++++++ >>> include/linux/security.h | 1 + >>> security/lockdown/lockdown.c | 1 + >>> 3 files changed, 8 insertions(+) >>> >>> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c >>> index f6d1287c7b83..fdf030e53035 100644 >>> --- a/drivers/staging/comedi/comedi_fops.c >>> +++ b/drivers/staging/comedi/comedi_fops.c >>> @@ -27,6 +27,7 @@ >>> >>> #include <linux/io.h> >>> #include <linux/uaccess.h> >>> +#include <linux/security.h> >>> >>> #include "comedi_internal.h" >>> >>> @@ -813,11 +814,16 @@ static int do_devconfig_ioctl(struct comedi_device *dev, >>> struct comedi_devconfig __user *arg) >>> { >>> struct comedi_devconfig it; >>> + int ret; >>> >>> lockdep_assert_held(&dev->mutex); >>> if (!capable(CAP_SYS_ADMIN)) >>> return -EPERM; >>> >>> + ret = security_locked_down(LOCKDOWN_COMEDI_DEVCONFIG); >>> + if (ret) >>> + return ret; >>> + >> >> You might consider moving that check to be done after the following 'if >> (!arg)' block, since that should be safe. (It detaches an already >> configured device from the comedi core.) > [...] > > How would it have been configured, though? It works on automatically registered comedi devices too. I suppose that could be done via the "unbind" file in the driver, but that goes through a different path and is a bit harder to use.
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index f6d1287c7b83..fdf030e53035 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -27,6 +27,7 @@ #include <linux/io.h> #include <linux/uaccess.h> +#include <linux/security.h> #include "comedi_internal.h" @@ -813,11 +814,16 @@ static int do_devconfig_ioctl(struct comedi_device *dev, struct comedi_devconfig __user *arg) { struct comedi_devconfig it; + int ret; lockdep_assert_held(&dev->mutex); if (!capable(CAP_SYS_ADMIN)) return -EPERM; + ret = security_locked_down(LOCKDOWN_COMEDI_DEVCONFIG); + if (ret) + return ret; + if (!arg) { if (is_device_busy(dev)) return -EBUSY; diff --git a/include/linux/security.h b/include/linux/security.h index 429f9f03372b..b16365dccfc5 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -113,6 +113,7 @@ enum lockdown_reason { LOCKDOWN_ACPI_TABLES, LOCKDOWN_PCMCIA_CIS, LOCKDOWN_TIOCSSERIAL, + LOCKDOWN_COMEDI_DEVCONFIG, LOCKDOWN_MODULE_PARAMETERS, LOCKDOWN_MMIOTRACE, LOCKDOWN_DEBUGFS, diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c index 0068cec77c05..971bb99b9051 100644 --- a/security/lockdown/lockdown.c +++ b/security/lockdown/lockdown.c @@ -28,6 +28,7 @@ static char *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = { [LOCKDOWN_ACPI_TABLES] = "modifying ACPI tables", [LOCKDOWN_PCMCIA_CIS] = "direct PCMCIA CIS storage", [LOCKDOWN_TIOCSSERIAL] = "reconfiguration of serial port IO", + [LOCKDOWN_COMEDI_DEVCONFIG] = "reconfiguration of Comedi legacy device", [LOCKDOWN_MODULE_PARAMETERS] = "unsafe module parameters", [LOCKDOWN_MMIOTRACE] = "unsafe mmio", [LOCKDOWN_DEBUGFS] = "debugfs access",
The COMEDI_DEVCONFIG ioctl can be used to configure I/O addresses and other hardware settings for non plug-and-play devices such as ISA cards. This should be disabled to preserve the kernel's integrity when it is locked down. Signed-off-by: Ben Hutchings <ben@decadent.org.uk> Cc: Matthew Garrett <mjg59@google.com> Cc: David Howells <dhowells@redhat.com> Cc: Ian Abbott <abbotti@mev.co.uk> Cc: H Hartley Sweeten <hsweeten@visionengravers.com> --- drivers/staging/comedi/comedi_fops.c | 6 ++++++ include/linux/security.h | 1 + security/lockdown/lockdown.c | 1 + 3 files changed, 8 insertions(+)