netdev - Re: [PATCH 3/3] vsockmon: Add virtio vsock hooks

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: <29e24e4f-321b-96ba-4ed5-4e401bed7d4a@deic.uab.cat>
Date:	Fri, 12 Aug 2016 16:18:54 +0200
From:	Gerard Garcia <ggarcia@...a.uab.cat>
To:	Stefan Hajnoczi <stefanha@...hat.com>
Cc:	netdev@...r.kernel.org, jhansen@...are.com, mst@...hat.com
Subject: Re: [PATCH 3/3] vsockmon: Add virtio vsock hooks

On 08/10/2016 01:59 PM, Stefan Hajnoczi wrote:
> On Mon, Aug 08, 2016 at 06:14:42PM +0200, ggarcia@...a.uab.cat wrote:
>> +static struct sk_buff *
>> +virtio_vsock_pkt_vsockmon_to_vsockmon_skb(struct virtio_vsock_pkt *pkt)
>> +{
>> +	struct sk_buff *skb;
>> +	struct af_vsockmon_hdr *hdr;
>> +	void *payload;
>> +
>> +	u32 skb_len = sizeof(struct af_vsockmon_hdr) + pkt->len;
>> +
>> +	skb = alloc_skb(skb_len, GFP_ATOMIC);
>> +	if (!skb)
>> +		return NULL;
>> +
>> +	hdr = (struct af_vsockmon_hdr *) skb_put(skb, sizeof(*hdr));
>> +
>> +	hdr->src_cid = pkt->hdr.src_cid;
>> +	hdr->src_port = pkt->hdr.src_port;
>> +	hdr->dst_cid = pkt->hdr.dst_cid;
>> +	hdr->dst_port = pkt->hdr.dst_port;
>> +	hdr->t = cpu_to_le16(AF_VSOCK_T_VIRTIO);
>> +	hdr->len = cpu_to_le16(sizeof(hdr->t_hdr));
>> +
>> +	switch(pkt->hdr.op) {
>
> Missing le16_to_cpu()
>

Right

>> +	case VIRTIO_VSOCK_OP_REQUEST:
>> +	case VIRTIO_VSOCK_OP_RESPONSE:
>> +		hdr->op = cpu_to_le16(AF_VSOCK_OP_CONNECT);
>> +		break;
>> +	case VIRTIO_VSOCK_OP_RST:
>> +	case VIRTIO_VSOCK_OP_SHUTDOWN:
>> +		hdr->op = cpu_to_le16(AF_VSOCK_OP_DISCONNECT);
>> +		break;
>> +	case VIRTIO_VSOCK_OP_RW:
>> +		hdr->op = cpu_to_le16(AF_VSOCK_OP_PAYLOAD);
>> +		break;
>> +	case VIRTIO_VSOCK_OP_CREDIT_UPDATE:
>> +	case VIRTIO_VSOCK_OP_CREDIT_REQUEST:
>> +		hdr->op = cpu_to_le16(AF_VSOCK_OP_CONTROL);
>> +		break;
>> +	default:
>> +		hdr->op = cpu_to_le16(AF_VSOCK_OP_UNKNOWN);
>> +		break;
>> +	}
>> +
>> +	hdr->t_hdr.virtio_hdr = pkt->hdr;
>> +
>> +	if (pkt->len) {
>> +		payload = skb_put(skb, pkt->len);
>> +		memcpy(payload, pkt->buf, pkt->len);
>> +	}
>> +
>> +	return skb;
>> +}
>> +
>> +static void vsock_deliver_tap_pkt(struct virtio_vsock_pkt *pkt)
>> +{
>> +	struct sk_buff *skb = virtio_vsock_pkt_to_vsockmon_skb(pkt);
>> +	if (skb) {
>> +		vsock_deliver_tap(skb);
>> +		kfree_skb(skb);
>
> Should this be consume_skb()?  The function's doc comment says:
>
>  *	Functions identically to kfree_skb, but kfree_skb assumes that the frame
>  *	is being dropped after a failure and notes that
>
> This isn't a failure case so kfree_skb() is not the right function.
>

I agree, consume_skb is more appropriate.