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

[Ian Abbott]

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 at decadent.org.uk>
>>> Cc: Matthew Garrett <mjg59 at google.com>
>>> Cc: David Howells <dhowells at redhat.com>
>>> Cc: Ian Abbott <abbotti at mev.co.uk>
>>> Cc: H Hartley Sweeten <hsweeten at 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.

-- 
-=( Ian Abbott <abbotti at mev.co.uk> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:    )=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-