From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751546AbdJDKk6 (ORCPT ); Wed, 4 Oct 2017 06:40:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50198 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751114AbdJDKk5 (ORCPT ); Wed, 4 Oct 2017 06:40:57 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6BD1D806B4 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=hdegoede@redhat.com Subject: Re: [PATCH] virt: Add vboxguest driver for Virtual Box Guest integration To: Greg Kroah-Hartman , Arnd Bergmann Cc: Christoph Hellwig , Michael Thayer , "Knut St . Osmundsen" , Larry Finger , Linux Kernel Mailing List References: <20171003092115.11341-1-hdegoede@redhat.com> <20171003092115.11341-2-hdegoede@redhat.com> <20171003100449.GA5491@infradead.org> <20171004101148.GA30855@kroah.com> <20171004103034.GA30246@kroah.com> From: Hans de Goede Message-ID: <6ff67ff3-5282-5131-39d0-ad5edf40dccd@redhat.com> Date: Wed, 4 Oct 2017 12:40:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171004103034.GA30246@kroah.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Wed, 04 Oct 2017 10:40:57 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> 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: >>>> >>>> >>>> >>>>>>> +#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