linux-kernel - Re: [RFC 1/2] misc: Add vboxguest driver for Virtual Box Guest integration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Aug 2017 09:38:16 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Arnd Bergmann <arnd@...db.de>,
        Michael Thayer <michael.thayer@...cle.com>,
        "Knut St . Osmundsen" <knut.osmundsen@...cle.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Larry Finger <Larry.Finger@...inger.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux FS-devel Mailing List <linux-fsdevel@...r.kernel.org>
Subject: Re: [RFC 1/2] misc: Add vboxguest driver for Virtual Box Guest
 integration

Hi,

On 12-08-17 23:56, Hans de Goede wrote:
> On 11-08-17 23:23, Arnd Bergmann wrote:

<snip>

>>> +/**
>>> + * @name VBoxGuest IOCTL codes and structures.
>>> + *
>>> + * The range 0..15 is for basic driver communication.
>>> + * The range 16..31 is for HGCM communication.
>>> + * The range 32..47 is reserved for future use.
>>> + * The range 48..63 is for OS specific communication.
>>> + * The 7th bit is reserved for future hacks.
>>> + * The 8th bit is reserved for distinguishing between 32-bit and 64-bit
>>> + * processes in future 64-bit guest additions.
>>> + * @{
>>> + */
>>> +#if __BITS_PER_LONG == 64
>>> +#define VBOXGUEST_IOCTL_FLAG           128
>>> +#else
>>> +#define VBOXGUEST_IOCTL_FLAG           0
>>> +#endif
>>> +/** @} */
>>
>> This seems misguided, we already know the difference between
>> 32-bit and 64-bit tasks based on the entry point, and if the
>> interface is properly designed then we don't care about this
>> difference at all.
> 
> I guess (and really guess at that) that this is there because on
> some guest OS-es supported by vbox the distinction between a
> 32 bit or 64 bit app calling the ioctl cannot be made in another
> way.
> 
> But I would not mind dropping this flag for linux.
> Michael / Knut would something like this be acceptable ?
> 
> --- a/include/VBox/VBoxGuest.h
> +++ b/include/VBox/VBoxGuest.h
> @@ -123,9 +123,9 @@
>    *          used.
>    * @{
>    */
> -#if defined(RT_ARCH_AMD64) || defined(RT_ARCH_SPARC64)
> +#if (defined(RT_ARCH_AMD64) || defined(RT_ARCH_SPARC64)) && !defined(RT_OS_LINUX)
>   # define VBOXGUEST_IOCTL_FLAG     128
> -#elif defined(RT_ARCH_X86) || defined(RT_ARCH_SPARC)
> +#elif defined(RT_ARCH_X86) || defined(RT_ARCH_SPARC) || defined(RT_OS_LINUX)
>   # define VBOXGUEST_IOCTL_FLAG     0
>   #else
>   # error "dunno which arch this is!"
> 
> (extend with some other changes such as properly adding a .compat_ioctl
> file-op ?

So I've extended the above into a proper patch to upstream
vbox svn (attached for reference). One thing which I've
learned while doing that is that Linux seems to be unique
with its compat_ioctl callback. Neither the BSDs nor Windows
has this, they all use different ioctl numbers for 32 / 64bit,
see e.g. :

https://tsyrklevich.net/clang_analyzer/freebsd_030917/report-de8f10.html

Which deals with 32 bit ioctl compat on freebsd, note
all the ioctls checked for are post_fixed with _32.

So given the cross-platform nature of the vboxguest ioctl
API I think we may end up having to stick with the (small)
VBOXGUEST_IOCTL_FLAG wart in the API. Because I certainly
don't want to break ABI compat between the mainline version
and the out of tree version.

But first lets see what vbox upstream has to say about my
patch to always make VBOXGUEST_IOCTL_FLAG 0 under Linux.

Regards,

Hans

View attachment "0001-VGDrvCommonIoCtl-Add-f32bit-flag-argument.patch" of type "text/x-patch" (18967 bytes)