Re: [PATCH 1/2] staging: comedi: Restrict COMEDI_DEVCONFIG when the kernel is locked down

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 2359 bytes --]

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.

-- 
Ben Hutchings
You can't have everything.  Where would you put it?



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]