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
| ||
|
Date: Mon, 14 Dec 2020 18:18:50 +0200 From: "Paraschiv, Andra-Irina" <andraprs@...zon.com> To: Stefano Garzarella <sgarzare@...hat.com>, Jakub Kicinski <kuba@...nel.org> CC: netdev <netdev@...r.kernel.org>, linux-kernel <linux-kernel@...r.kernel.org>, "David S . Miller" <davem@...emloft.net>, David Duncan <davdunc@...zon.com>, Dexuan Cui <decui@...rosoft.com>, Alexander Graf <graf@...zon.de>, Jorgen Hansen <jhansen@...are.com>, Stefan Hajnoczi <stefanha@...hat.com>, Vitaly Kuznetsov <vkuznets@...hat.com> Subject: Re: [PATCH net-next v3 0/4] vsock: Add flags field in the vsock address On 14/12/2020 10:13, Stefano Garzarella wrote: > > On Sat, Dec 12, 2020 at 09:16:08AM -0800, Jakub Kicinski wrote: >> On Fri, 11 Dec 2020 16:24:13 +0100 Stefano Garzarella wrote: >>> On Fri, Dec 11, 2020 at 12:32:37PM +0200, Andra Paraschiv wrote: >>> >vsock enables communication between virtual machines and the host >>> they are >>> >running on. Nested VMs can be setup to use vsock channels, as the >>> multi >>> >transport support has been available in the mainline since the v5.5 >>> Linux kernel >>> >has been released. >>> > >>> >Implicitly, if no host->guest vsock transport is loaded, all the >>> vsock packets >>> >are forwarded to the host. This behavior can be used to setup >>> communication >>> >channels between sibling VMs that are running on the same host. One >>> example can >>> >be the vsock channels that can be established within AWS Nitro >>> Enclaves >>> >(see Documentation/virt/ne_overview.rst). >>> > >>> >To be able to explicitly mark a connection as being used for a >>> certain use case, >>> >add a flags field in the vsock address data structure. The value of >>> the flags >>> >field is taken into consideration when the vsock transport is >>> assigned. This way >>> >can distinguish between different use cases, such as nested VMs / >>> local >>> >communication and sibling VMs. >>> > >>> >The flags field can be set in the user space application connect >>> logic. On the >>> >listen path, the field can be set in the kernel space logic. >>> > >>> >>> I reviewed all the patches and they are in a good shape! >>> >>> Maybe the last thing to add is a flags check in the >>> vsock_addr_validate(), to avoid that flags that we don't know how to >>> handle are specified. >>> For example if in the future we add new flags that this version of the >>> kernel is not able to satisfy, we should return an error to the >>> application. >>> >>> I mean something like this: >>> >>> diff --git a/net/vmw_vsock/vsock_addr.c >>> b/net/vmw_vsock/vsock_addr.c >>> index 909de26cb0e7..73bb1d2fa526 100644 >>> --- a/net/vmw_vsock/vsock_addr.c >>> +++ b/net/vmw_vsock/vsock_addr.c >>> @@ -22,6 +22,8 @@ EXPORT_SYMBOL_GPL(vsock_addr_init); >>> >>> int vsock_addr_validate(const struct sockaddr_vm *addr) >>> { >>> + unsigned short svm_valid_flags = VMADDR_FLAG_TO_HOST; >>> + >>> if (!addr) >>> return -EFAULT; >>> >>> @@ -31,6 +33,9 @@ int vsock_addr_validate(const struct >>> sockaddr_vm *addr) >>> if (addr->svm_zero[0] != 0) >>> return -EINVAL; >> >> Strictly speaking this check should be superseded by the check below >> (AKA removed). We used to check svm_zero[0], with the new field added >> this now checks svm_zero[2]. Old applications may have not initialized >> svm_zero[2] (we're talking about binary compatibility here, apps built >> with old headers). >> >>> + if (addr->svm_flags & ~svm_valid_flags) >>> + return -EINVAL; >> >> The flags should also probably be one byte (we can define a "more >> flags" flag to unlock further bytes) - otherwise on big endian the >> new flag will fall into svm_zero[1] so the v3 improvements are moot >> for big endian, right? > > Right, I assumed the entire svm_zero[] was zeroed out, but we can't be > sure. > > So, I agree to change the svm_flags to 1 byte (__u8), and remove the > superseded check that you pointed out. > With these changes we should be fully binary compatibility. > Here we go, sent out v4: https://lore.kernel.org/lkml/20201214161122.37717-1-andraprs@amazon.com/ Thank you both. Andra >> >>> return 0; >>> } >>> EXPORT_SYMBOL_GPL(vsock_addr_validate); >> > Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
Powered by blists - more mailing lists