netdev - Re: [PATCH net-next v5 4/4] vsock/virtio: MSG

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] [day] [month] [year] [list]

Message-ID: <0e1a5faf-88b7-87c5-ff92-413991878894@gmail.com>
Date: Sat, 5 Aug 2023 14:12:33 +0300
From: Arseniy Krasnov <oxffffaa@...il.com>
To: Paolo Abeni <pabeni@...hat.com>,
 Arseniy Krasnov <AVKrasnov@...rdevices.ru>,
 Stefan Hajnoczi <stefanha@...hat.com>,
 Stefano Garzarella <sgarzare@...hat.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, "Michael S. Tsirkin" <mst@...hat.com>,
 Jason Wang <jasowang@...hat.com>,
 Bobby Eshleman <bobby.eshleman@...edance.com>
Cc: kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org, kernel@...rdevices.ru
Subject: Re: [PATCH net-next v5 4/4] vsock/virtio: MSG_ZEROCOPY flag support

On 01.08.2023 16:34, Paolo Abeni wrote:
> On Sun, 2023-07-30 at 11:59 +0300, Arseniy Krasnov wrote:
>> +static int virtio_transport_fill_skb(struct sk_buff *skb,
>> +				     struct virtio_vsock_pkt_info *info,
>> +				     size_t len,
>> +				     bool zcopy)
>> +{
>> +	if (zcopy) {
>> +		return __zerocopy_sg_from_iter(info->msg, NULL, skb,
>> +					      &info->msg->msg_iter,
>> +					      len);
>> +	} else {
> 
> 
> No need for an else statement after 'return'
> 
>> +		void *payload;
>> +		int err;
>> +
>> +		payload = skb_put(skb, len);
>> +		err = memcpy_from_msg(payload, info->msg, len);
>> +		if (err)
>> +			return -1;
>> +
>> +		if (msg_data_left(info->msg))
>> +			return 0;
>> +
> 
> This path does not update truesize, evem if it increases the skb len...

Sorry, but what is potential problem here ? In this path I copy data from the user's
buffer to the linear skb (there is no fragged part in this case). I think 'truesize'
is constant in this case - it is SKB_TRUESIZE(length of skb buffer) - there is no need
to update it as 'truesize' does not show amount of data in skb, only real size of
skb's buffer.

For non-linear case, __zerocopy_sg_from_iter() always updates 'sk_wmem_alloc' of the
socket during iterating over frags array.

Also 'skb_set_owner_w()' is called before this code, thus setting 'sk_wmem_alloc' to the
'truesize' value of the skb.

Thanks, Arseniy

> 
>> +		return 0;
>> +	}
>> +}
> 
> [...]
> 
>> @@ -214,6 +251,70 @@ static u16 virtio_transport_get_type(struct sock *sk)
>>  		return VIRTIO_VSOCK_TYPE_SEQPACKET;
>>  }
>>  
>> +static struct sk_buff *virtio_transport_alloc_skb(struct vsock_sock *vsk,
>> +						  struct virtio_vsock_pkt_info *info,
>> +						  size_t payload_len,
>> +						  bool zcopy,
>> +						  u32 src_cid,
>> +						  u32 src_port,
>> +						  u32 dst_cid,
>> +						  u32 dst_port)
>> +{
>> +	struct sk_buff *skb;
>> +	size_t skb_len;
>> +
>> +	skb_len = VIRTIO_VSOCK_SKB_HEADROOM;
>> +
>> +	if (!zcopy)
>> +		skb_len += payload_len;
>> +
>> +	skb = virtio_vsock_alloc_skb(skb_len, GFP_KERNEL);
>> +	if (!skb)
>> +		return NULL;
>> +
>> +	virtio_transport_init_hdr(skb, info, src_cid, src_port,
>> +				  dst_cid, dst_port,
>> +				  payload_len);
>> +
>> +	/* Set owner here, because '__zerocopy_sg_from_iter()' uses
>> +	 * owner of skb without check to update 'sk_wmem_alloc'.
>> +	 */
>> +	if (vsk)
>> +		skb_set_owner_w(skb, sk_vsock(vsk));
> 
> ... which can lead to bad things(TM) if the skb goes trough some later
> non trivial processing, due to the above skb_set_owner_w().
> 
> Additionally can be the following condition be true:
> 
> 	vsk == NULL && (info->msg && payload_len > 0) && zcopy
> 
> ???
> 
> If so it looks like skb can go through __zerocopy_sg_from_iter() even
> without a prior skb_set_owner_w()...
> 
> 
> Cheers,
> 
> Paolo
>