[1/2] staging: comedi: Restrict COMEDI_DEVCONFIG when the kernel is locked down
diff mbox series

Message ID 20190830154549.vss6h5tlrl6d5r5y@decadent.org.uk
State New
Headers show
Series
  • [1/2] staging: comedi: Restrict COMEDI_DEVCONFIG when the kernel is locked down
Related show

Commit Message

Ben Hutchings Aug. 30, 2019, 3:45 p.m. UTC
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(+)

Comments

Ian Abbott Aug. 30, 2019, 5:35 p.m. UTC | #1
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",
>
Ben Hutchings Aug. 31, 2019, 9:50 a.m. UTC | #2
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.
Ian Abbott Sept. 2, 2019, 9:26 a.m. UTC | #3
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.

Patch
diff mbox series

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",