Re: [PATCH] virt: vbox: Implement passing requestor info to the host for VirtualBox 6.0.x

[Hans de Goede]

Hi,

On 20-03-19 19:26, Greg Kroah-Hartman wrote:
> On Wed, Mar 20, 2019 at 10:35:19AM +0100, Hans de Goede wrote:
> > diff --git a/drivers/virt/vboxguest/vboxguest_version.h b/drivers/virt/vboxguest/vboxguest_version.h
> > index 77f0c8f8a231..84834dad38d5 100644
> > --- a/drivers/virt/vboxguest/vboxguest_version.h
> > +++ b/drivers/virt/vboxguest/vboxguest_version.h
> > @@ -9,11 +9,10 @@
> >   #ifndef __VBOX_VERSION_H__
> >   #define __VBOX_VERSION_H__
> >   
> > -/* Last synced October 4th 2017 */
>
> We don't care about the date sync anymore?

I did not sync with the latest version, I've picked
the svn revision number from the 6.0.0 release
(6.0.4 is out now) in case one of the 6.0.x releases
have introduced something new which we do not implement
yet.

> > -#define VBG_VERSION_MAJOR 5
> > -#define VBG_VERSION_MINOR 2
> > +#define VBG_VERSION_MAJOR 6
> > +#define VBG_VERSION_MINOR 0
> >   #define VBG_VERSION_BUILD 0
> > -#define VBG_SVN_REV 68940
> > -#define VBG_VERSION_STRING "5.2.0"
> > +#define VBG_SVN_REV 127566
> > +#define VBG_VERSION_STRING "6.0.0"
>
> Do we really need to keep track of this?

Unfortunately yes, last time I checked the host has
some code checking the VBG_SVN_REV to determine whether
or not to apply some bug workaround.

> >   
> >   #endif
> > diff --git a/drivers/virt/vboxguest/vmmdev.h b/drivers/virt/vboxguest/vmmdev.h
> > index 5e2ae978935d..6337b8d75d96 100644
> > --- a/drivers/virt/vboxguest/vmmdev.h
> > +++ b/drivers/virt/vboxguest/vmmdev.h
> > @@ -98,8 +98,8 @@ struct vmmdev_request_header {
> >   	s32 rc;
> >   	/** Reserved field no.1. MBZ. */
> >   	u32 reserved1;
> > -	/** Reserved field no.2. MBZ. */
> > -	u32 reserved2;
> > +	/** IN: Requestor information (VMMDEV_REQUESTOR_*) */
> > +	u32 requestor;
>
> They didn't use the first reserved field?  Oh well :(

There is another header for a hgcm-call which partly mirrors
this one and there they are using reserved1, I think it has
something to do with this.
> > --- a/include/uapi/linux/vbox_vmmdev_types.h
> > +++ b/include/uapi/linux/vbox_vmmdev_types.h
> > @@ -102,6 +102,37 @@ enum vmmdev_request_type {
> >   #define VMMDEVREQ_HGCM_CALL VMMDEVREQ_HGCM_CALL32
> >   #endif
> >   
> > +/* vmmdev_request_header.requestor defines */
> > +
> > +/* Requestor user not given. */
> > +#define VMMDEV_REQUESTOR_USR_NOT_GIVEN                      0x00000000
> > +/* The kernel driver (VBoxGuest) is the requestor. */
> > +#define VMMDEV_REQUESTOR_USR_DRV                            0x00000001
> > +/* Some other kernel driver is the requestor. */
> > +#define VMMDEV_REQUESTOR_USR_DRV_OTHER                      0x00000002
> > +/* The root or a admin user is the requestor. */
> > +#define VMMDEV_REQUESTOR_USR_ROOT                           0x00000003
> > +/* Regular joe user is making the request. */
> > +#define VMMDEV_REQUESTOR_USR_USER                           0x00000006
> > +/* User classification mask. */
> > +#define VMMDEV_REQUESTOR_USR_MASK                           0x00000007
> > +/* Kernel mode request. */
> > +#define VMMDEV_REQUESTOR_KERNEL                             0x00000000
>
> Wait, isn't that the same as VMMDEV_REQUESTOR_USR_NOT_GIVEN?

The call coming directly from the call, rather then through the
/dev/vboxguest or /dev/vboxuser devices is indicated by the
lack of the VMMDEV_REQUESTOR_USERMODE bit. I will add a

#define VMMDEV_REQUESTOR_MODE_MASK				0x00000008

To make this more clear for v2 and add blank lines to group the different
items together per mask.


> > +/* User mode request. */
> > +#define VMMDEV_REQUESTOR_USERMODE                           0x00000008
> > +/* Don't know the physical console association of the requestor. */
> > +#define VMMDEV_REQUESTOR_CON_DONT_KNOW                      0x00000000
>
> And here, why is this number recycled?

VMMDEV_REQUESTOR_USR_NOT_GIVEN (the first 0x0000000 define) is part of the
which user made this call mask: VMMDEV_REQUESTOR_USR_MASK, this is part
of the is the user behind the physical console info mask:
VMMDEV_REQUESTOR_CON_MASK

I've omitted the other defines since we are not using them, I will add
them to make this more clear.


> > +/* Console classification mask. */
> > +#define VMMDEV_REQUESTOR_CON_MASK                           0x00000040
> > +/* Requestor is member of special VirtualBox user group. */
> > +#define VMMDEV_REQUESTOR_GRP_VBOX                           0x00000080
>
> Can you add a blank line here to make it more obvious it is "TRUST"
> stuff here?

Ack.


> > +/* Requestor trust level: Unspecified */
> > +#define VMMDEV_REQUESTOR_TRUST_NOT_GIVEN                    0x00000000
> > +/* Requestor trust level mask */
> > +#define VMMDEV_REQUESTOR_TRUST_MASK                         0x00007000
>
> So those bits are the "trust" values?

Right, again I did not add defines for values we do not use under Linux
(this value comes from some Windows authentication mechanism, so under Linux
it is always VMMDEV_REQUESTOR_TRUST_NOT_GIVEN)


> that's odd, oh well, it's their api, not ours :(
>
> > +/* Requestor is using the less trusted user device node (/dev/vboxuser) */
> > +#define VMMDEV_REQUESTOR_USER_DEVICE                        0x00008000
>
> Wait, what is this for?
>
> So the dev node isn't as "trusted" as some other api?  Why not?

There are 2 device nodes:

/dev/vboxguest
/dev/vboxuser

With the latter being mode 0666, the vboxguest code already disallows
some requests / IOCTLs when done on the /dev/vboxuser node.

With this new requestor mechanism, the purpose of which is to allow host
admins to write finer grained policies for which requests to accept from the
guest AFAIK, we are now also forwarding the info that the request was done
on the world accessible /dev/vboxuser node, rather then on the restricted
/dev/vboxguest node to the host.

Regards,

Hans