netdev - Re: [PATCH 03/10] vsock: owner field is specific to VMCI

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:   Wed, 27 Oct 2021 08:13:49 +0000
From:   Jorgen Hansen <jhansen@...are.com>
To:     Stefano Garzarella <sgarzare@...hat.com>
CC:     Marc-André Lureau <marcandre.lureau@...hat.com>,
        netdev <netdev@...r.kernel.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH 03/10] vsock: owner field is specific to VMCI


> On 26 Oct 2021, at 13:16, Stefano Garzarella <sgarzare@...hat.com> wrote:
> 
> CCing Jorgen.
> 
> On Thu, Oct 21, 2021 at 04:37:07PM +0400, Marc-André Lureau wrote:
>> This field isn't used by other transports.
> 
> If the field is used only in the VMCI transport, maybe it's better to 
> move the field and the code in that transport.

If the transport needs initialize these fields, that should happen when we
call vsock_assign_transport. So we would need to validate that
get_current_cred() gets the right credentials and that the parent of a
socket has an Initialised owner field at that point in time.

sock_assign_transport may be called when processing an
incoming packet when a remote connects to a listening socket,
and in that case, the owner will be based on the parent socket.
If the parent socket hasn’t been assigned a transport (and as I
remember it, that isn’t the case for a listening socket), then it
isn’t possible to initialize the owner field at this point using
the value from the parent. So the initialisation of the fields
probably have to stay in af_vsock.c as part of the generic structure.

Is there a particular reason to do this change as part of this series
of patches?

Thanks,
Jorgen

> Thanks,
> Stefano
> 
>> 
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@...hat.com>
>> ---
>> include/net/af_vsock.h   | 2 ++
>> net/vmw_vsock/af_vsock.c | 6 ++++++
>> 2 files changed, 8 insertions(+)
>> 
>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>> index ab207677e0a8..e626d9484bc5 100644
>> --- a/include/net/af_vsock.h
>> +++ b/include/net/af_vsock.h
>> @@ -41,7 +41,9 @@ struct vsock_sock {
>>                                       * cached peer?
>>                                       */
>>      u32 cached_peer;  /* Context ID of last dgram destination check. */
>> +#if IS_ENABLED(CONFIG_VMWARE_VMCI_VSOCKETS)
>>      const struct cred *owner;
>> +#endif
>>      /* Rest are SOCK_STREAM only. */
>>      long connect_timeout;
>>      /* Listening socket that this came from. */
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index e2c0cfb334d2..1925682a942a 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -761,7 +761,9 @@ static struct sock *__vsock_create(struct net *net,
>>      psk = parent ? vsock_sk(parent) : NULL;
>>      if (parent) {
>>              vsk->trusted = psk->trusted;
>> +#if IS_ENABLED(CONFIG_VMWARE_VMCI_VSOCKETS)
>>              vsk->owner = get_cred(psk->owner);
>> +#endif
>>              vsk->connect_timeout = psk->connect_timeout;
>>              vsk->buffer_size = psk->buffer_size;
>>              vsk->buffer_min_size = psk->buffer_min_size;
>> @@ -769,7 +771,9 @@ static struct sock *__vsock_create(struct net *net,
>>              security_sk_clone(parent, sk);
>>      } else {
>>              vsk->trusted = ns_capable_noaudit(&init_user_ns, CAP_NET_ADMIN);
>> +#if IS_ENABLED(CONFIG_VMWARE_VMCI_VSOCKETS)
>>              vsk->owner = get_current_cred();
>> +#endif
>>              vsk->connect_timeout = VSOCK_DEFAULT_CONNECT_TIMEOUT;
>>              vsk->buffer_size = VSOCK_DEFAULT_BUFFER_SIZE;
>>              vsk->buffer_min_size = VSOCK_DEFAULT_BUFFER_MIN_SIZE;
>> @@ -833,7 +837,9 @@ static void vsock_sk_destruct(struct sock *sk)
>>      vsock_addr_init(&vsk->local_addr, VMADDR_CID_ANY, VMADDR_PORT_ANY);
>>      vsock_addr_init(&vsk->remote_addr, VMADDR_CID_ANY, VMADDR_PORT_ANY);
>> 
>> +#if IS_ENABLED(CONFIG_VMWARE_VMCI_VSOCKETS)
>>      put_cred(vsk->owner);
>> +#endif
>> }
>> 
>> static int vsock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>> --
>> 2.33.0.721.g106298f7f9
>> 
>