Linux Virtualization - Nov 2023 - [PATCH v4 4/4] vduse: Add LSM hooks to check Virtio device type

On Thu, Nov 02, 2023 at 06:56:59PM +0100, Maxime Coquelin
wrote:
> 
> 
> On 10/24/23 17:30, Casey Schaufler wrote:
> > On 10/24/2023 2:49 AM, Maxime Coquelin wrote:
> > > 
> > > 
> > > On 10/23/23 17:13, Casey Schaufler wrote:
> > > > On 10/23/2023 12:28 AM, Maxime Coquelin wrote:
> > > > > 
> > > > > 
> > > > > On 10/21/23 00:20, Casey Schaufler wrote:
> > > > > > On 10/20/2023 8:58 AM, Maxime Coquelin wrote:
> > > > > > > This patch introduces LSM hooks for devices
creation,
> > > > > > > destruction and opening operations, checking
the
> > > > > > > application is allowed to perform these
operations for
> > > > > > > the Virtio device type.
> > > > > > 
> > > > > > Why do you think that there needs to be a special
LSM check for virtio
> > > > > > devices? What can't existing device attributes
be used?
> > > > > 
> > > > > Michael asked for a way for SELinux to allow/prevent
the creation of
> > > > > some types of devices [0].
> > > > > 
> > > > > A device is created using ioctl() on VDUSE control
chardev. Its type is
> > > > > specified via a field in the structure passed in
argument.
> > > > > 
> > > > > I didn't see other way than adding dedicated LSM
hooks to achieve this,
> > > > > but it is possible that their is a better way to do it?
> > > > 
> > > > At the very least the hook should be made more general, and
I'd have to
> > > > see a proposal before commenting on that.
security_dev_destroy(dev)
> > > > might
> > > > be a better approach. If there's reason to control
destruction of vduse
> > > > devices it's reasonable to assume that there are other
devices with the
> > > > same or similar properties.
> > > 
> > > VDUSE is different from other devices as the device is actually
> > > implemented by the user-space application, so this is very
specific in
> > > my opinion.
> > 
> > This is hardly unique. If you're implementing the device
> > in user-space you may well be able to implement the desired
> > controls there.
> > 
> > > 
> > > > 
> > > > Since SELinux is your target use case, can you explain why
you can't
> > > > create SELinux policy to enforce the restrictions you're
after? I
> > > > believe
> > > > (but can be proven wrong, of course) that SELinux has
mechanism for
> > > > dealing
> > > > with controls on ioctls.
> > > > 
> > > 
> > > I am not aware of such mechanism to deal with ioctl(), if you
have a
> > > pointer that would be welcome.
> > 
> > security/selinux/hooks.c
> 
> We might be able to extend selinux_file_ioctl(), but that will only
> covers the ioctl for the control file, this patch also adds hook for the
> device file opening that would need dedicated hook as the device type
> information is stored in the device's private data.
> 
> Michael, before going further, I would be interested in your feedback.
> Was this patch what you had in mind when requesting for a way to
> allow/deny devices types for a given application?
> 
> Regards,
> Maxime

Yes, this is more or less what I had in mind.
> > 
> > > 
> > > Thanks,
> > > Maxime
> > > 
> > > > 
> > > > > 
> > > > > Thanks,
> > > > > Maxime
> > > > > 
> > > > > [0]:
> > > > >
https://lore.kernel.org/all/20230829130430-mutt-send-email-mst at kernel.org/
> > > > > 
> > > > > 
> > > > 
> > > 
> >

On 11/2/23 19:59, Michael S. Tsirkin wrote:
> On Thu, Nov 02, 2023 at 06:56:59PM +0100, Maxime Coquelin wrote:
>>
>>
>> On 10/24/23 17:30, Casey Schaufler wrote:
>>> On 10/24/2023 2:49 AM, Maxime Coquelin wrote:
>>>>
>>>>
>>>> On 10/23/23 17:13, Casey Schaufler wrote:
>>>>> On 10/23/2023 12:28 AM, Maxime Coquelin wrote:
>>>>>>
>>>>>>
>>>>>> On 10/21/23 00:20, Casey Schaufler wrote:
>>>>>>> On 10/20/2023 8:58 AM, Maxime Coquelin wrote:
>>>>>>>> This patch introduces LSM hooks for devices
creation,
>>>>>>>> destruction and opening operations, checking
the
>>>>>>>> application is allowed to perform these
operations for
>>>>>>>> the Virtio device type.
>>>>>>>
>>>>>>> Why do you think that there needs to be a special
LSM check for virtio
>>>>>>> devices? What can't existing device attributes
be used?
>>>>>>
>>>>>> Michael asked for a way for SELinux to allow/prevent
the creation of
>>>>>> some types of devices [0].
>>>>>>
>>>>>> A device is created using ioctl() on VDUSE control
chardev. Its type is
>>>>>> specified via a field in the structure passed in
argument.
>>>>>>
>>>>>> I didn't see other way than adding dedicated LSM
hooks to achieve this,
>>>>>> but it is possible that their is a better way to do it?
>>>>>
>>>>> At the very least the hook should be made more general, and
I'd have to
>>>>> see a proposal before commenting on that.
security_dev_destroy(dev)
>>>>> might
>>>>> be a better approach. If there's reason to control
destruction of vduse
>>>>> devices it's reasonable to assume that there are other
devices with the
>>>>> same or similar properties.
>>>>
>>>> VDUSE is different from other devices as the device is actually
>>>> implemented by the user-space application, so this is very
specific in
>>>> my opinion.
>>>
>>> This is hardly unique. If you're implementing the device
>>> in user-space you may well be able to implement the desired
>>> controls there.
>>>
>>>>
>>>>>
>>>>> Since SELinux is your target use case, can you explain why
you can't
>>>>> create SELinux policy to enforce the restrictions
you're after? I
>>>>> believe
>>>>> (but can be proven wrong, of course) that SELinux has
mechanism for
>>>>> dealing
>>>>> with controls on ioctls.
>>>>>
>>>>
>>>> I am not aware of such mechanism to deal with ioctl(), if you
have a
>>>> pointer that would be welcome.
>>>
>>> security/selinux/hooks.c
>>
>> We might be able to extend selinux_file_ioctl(), but that will only
>> covers the ioctl for the control file, this patch also adds hook for
the
>> device file opening that would need dedicated hook as the device type
>> information is stored in the device's private data.
>>
>> Michael, before going further, I would be interested in your feedback.
>> Was this patch what you had in mind when requesting for a way to
>> allow/deny devices types for a given application?
>>
>> Regards,
>> Maxime
> 
> 
> Yes, this is more or less what I had in mind.
Great.

Do you think we need to cover both ioctl() on the control file and
open() on the device file, or only ioctl() is enough?

If the former, we will need VDUSE-specific hooks. I may be able to
improve my patch to have a single hook instead of 3 by passing the type
of operation as an extra argument (create/destroy/open).

If the latter, we may be able to extend the generic ioctl hook.

Personally, I think it would make sense to also ensure a given
application can only open existing VDUSE devices it supports. For
example, openvswitch should only be allowed to open networking VDUSE
devices.

Thanks,
Maxime
> 
>>>
>>>>
>>>> Thanks,
>>>> Maxime
>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Maxime
>>>>>>
>>>>>> [0]:
>>>>>>
https://lore.kernel.org/all/20230829130430-mutt-send-email-mst at kernel.org/
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>