linux-kernel - Re: [PATCH v6 13/21] gunyah: vm_mgr: Introduce basic VM Manager

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]

Message-ID: <ab7184f6-09e4-4b1c-bebe-30119b8da46c@quicinc.com>
Date:   Thu, 3 Nov 2022 15:33:22 -0700
From:   Elliot Berman <quic_eberman@...cinc.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Arnd Bergmann <arnd@...db.de>,
        Bjorn Andersson <quic_bjorande@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Murali Nalajala <quic_mnalajal@...cinc.com>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>,
        Carl van Schaik <quic_cvanscha@...cinc.com>,
        Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>,
        Andy Gross <agross@...nel.org>,
        Jassi Brar <jassisinghbrar@...il.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        "Mark Rutland" <mark.rutland@....com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Sudeep Holla <sudeep.holla@....com>,
        "Marc Zyngier" <maz@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
        Jonathan Corbet <corbet@....net>,
        Will Deacon <will@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Amol Maheshwari <amahesh@....qualcomm.com>,
        Kalle Valo <kvalo@...nel.org>, <devicetree@...r.kernel.org>,
        <linux-doc@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 13/21] gunyah: vm_mgr: Introduce basic VM Manager



On 11/2/2022 5:20 PM, Greg Kroah-Hartman wrote:
> On Wed, Nov 02, 2022 at 11:44:51AM -0700, Elliot Berman wrote:
>>
>>
>> On 11/2/2022 12:31 AM, Arnd Bergmann wrote:
>>> On Wed, Oct 26, 2022, at 20:58, Elliot Berman wrote:
>>>
>>>> +static const struct file_operations gh_vm_fops = {
>>>> +	.unlocked_ioctl = gh_vm_ioctl,
>>>> +	.release = gh_vm_release,
>>>> +	.llseek = noop_llseek,
>>>> +};
>>>
>>> There should be a .compat_ioctl entry here, otherwise it is
>>> impossible to use from 32-bit tasks. If all commands have
>>> arguments passed through a pointer to a properly defined
>>> structure, you can just set it to compat_ptr_ioctl.
>>>
>>
>> Ack.
>>
>>>> +static long gh_dev_ioctl_create_vm(unsigned long arg)
>>>> +{
>>>> +	struct gunyah_vm *ghvm;
>>>> +	struct file *file;
>>>> +	int fd, err;
>>>> +
>>>> +	/* arg reserved for future use. */
>>>> +	if (arg)
>>>> +		return -EINVAL;
>>>
>>> Do you have something specific in mind here? If 'create'
>>> is the only command you support, and it has no arguments,
>>> it would be easier to do it implicitly during open() and
>>> have each fd opened from /dev/gunyah represent a new VM.
>>>
>>
>> I'd like the argument here to support different types of virtual machines. I
>> want to leave open what "different types" can be in case something new comes
>> up in the future, but immediately "different type" would correspond to a few
>> different authentication mechanisms for virtual machines that Gunyah
>> supports.
> 
> Please don't add code that does not actually do something now, as that
> makes it impossible to review properly, _AND_ no one knows what is going
> to happen in the future.  In the future, you can just add a new ioctl
> and all is good, no need to break working userspace by suddenly looking
> at the arg value and doing something with it.
> 

I think the argument does something today and it's documented to need to 
be 0. If a userspace from the future provides non-zero value, Linux will 
correctly reject it because it doesn't know how to interpret the 
different VM types.

I can document it more clearly as the VM type field and support only the 
one VM type today.

Creating new ioctl for each VM type feels to me like I didn't design 
CREATE_VM ioctl right in first place.

Thanks,
Elliot