Re: [PATCH] virt: Add vboxguest driver for Virtual Box Guest integration

[Hans de Goede]

Hi,

On 04-10-17 12:30, Greg Kroah-Hartman wrote:
> On Wed, Oct 04, 2017 at 12:23:41PM +0200, Arnd Bergmann wrote:
> > On Wed, Oct 4, 2017 at 12:11 PM, Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > On Wed, Oct 04, 2017 at 11:32:23AM +0200, Hans de Goede wrote:
> > > > Hi,
> > > >
> > > > On 03-10-17 13:41, Hans de Goede wrote:
> > > >
> > > > <snip>
> > > >
> > > > > > > +#define CHECK_IOCTL_IN(req)                             \
> > > > > > > +do {                                         \
> > > > > > > +    if ((req)->Hdr.cbIn != (sizeof((req)->Hdr) + sizeof((req)->u.In)) || \
> > > > > > > +        (req)->Hdr.cbOut != sizeof((req)->Hdr))                 \
> > > > > > > +        return -EINVAL;                             \
> > > > > > > +} while (0)
> > > > > >
> > > > > > Make these things functions instead of macros.
> > > > >
> > > > > Turning these into functions is a good idea I will do so for v2.
> > > >
> > > > Correction, I forgot that the passed in "req" macro
> > > > argument has a different type with all the calls, so
> > > > these cannot be changed into functions because they
> > > > rely on sizeof on the specific type to do the size
> > > > checks.
> > >
> > > Don't we already have built-in checks for these types of things?  Surely
> > > we don't require each ioctl user in the kernel to do this by
> > > themselves...
> >
> > No other driver uses this kind of header for the ioctl structures,
> > usually we just rely on the ioctl command number to encode the
> > size, or we copy a fixed length.
>
> Then why can't we do the same thing here as well?

VirtualBox uses the same ioctl interface for the guest-additions userspace
parts on all supported platforms and not all platforms support encoding
the size in the ioctl number, so all the ioctl data structs have a header
with the in and out sizes in there, these macros check that header.

As mentioned during the RFC discussions changing the ioctl interface
is going to be troublesome to do because we want to be ABI compatible
with the kernel module shipped with the guest additions.

Having 2 separate guest additions builds, one for running with the out
of tree driver (which eventually should go away I hope, but certainly not
soon) and another build for the mainline version of the vboxguest driver
is not supportable.

I'm confident that if we find issues during the review process, which
have security implications, or where behavior is not clearly specified,
that we can get VirtualBox upstream to fix the API for that, but outright
re-designing the API is not really an option I believe.

As for these specific macros to check the ioctl data struct header,
if these are considered a problem I can simply write out the code in
the few places where this macro is called.

Regards,

Hans