netdev - Re: [PATCH v4 3/3] VSOCK: Add virtio vsock vsockmon 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: <20170420103206.GB28129@stefanha-x1.localdomain>
Date:   Thu, 20 Apr 2017 11:32:06 +0100
From:   Stefan Hajnoczi <stefanha@...hat.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     netdev@...r.kernel.org, Zhu Yanjun <zyjzyj2000@...il.com>,
        Gerard Garcia <ggarcia@...a.uab.cat>,
        Jorgen Hansen <jhansen@...are.com>
Subject: Re: [PATCH v4 3/3] VSOCK: Add virtio vsock vsockmon hooks

On Thu, Apr 13, 2017 at 09:47:08PM +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 13, 2017 at 05:18:11PM +0100, Stefan Hajnoczi wrote:
> > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > index af087b4..aae60c1 100644
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/virtio_ids.h>
> >  #include <linux/virtio_config.h>
> >  #include <linux/virtio_vsock.h>
> > +#include <uapi/linux/vsockmon.h>
> >  
> >  #include <net/sock.h>
> >  #include <net/af_vsock.h>
> > @@ -85,6 +86,63 @@ virtio_transport_alloc_pkt(struct virtio_vsock_pkt_info *info,
> >  	return NULL;
> >  }
> >  
> > +/* Packet capture */
> > +void virtio_transport_deliver_tap_pkt(struct virtio_vsock_pkt *pkt)
> > +{
> > +	struct sk_buff *skb;
> > +	struct af_vsockmon_hdr *hdr;
> > +	unsigned char *t_hdr, *payload;
> > +
> > +	skb = alloc_skb(sizeof(*hdr) + sizeof(pkt->hdr) + pkt->len,
> > +			GFP_ATOMIC);
> > +	if (!skb)
> > +		return; /* nevermind if we cannot capture the packet */
> > +
> > +	hdr = (struct af_vsockmon_hdr *)skb_put(skb, sizeof(*hdr));
> > +
> > +	/* pkt->hdr is little-endian so no need to byteswap here */
> 
> Comment does not seem to make sense. Drop it?

All fields in pkt->hdr are little-endian.  All fields in the
af_vsockmon_hdr are little-endian too.

Normally code operating on either of these structs byteswaps the fields.
Therefore it seemed worth a comment to clarify that it's okay to omit
byteswaps.  The comment will help anyone auditing the code for
endianness bugs.

Why do you say it doesn't make sense?

> > +	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->transport = cpu_to_le16(AF_VSOCK_TRANSPORT_VIRTIO);
> > +	hdr->len = cpu_to_le16(sizeof(pkt->hdr));
> > +	hdr->reserved[0] = hdr->reserved[1] = 0;
> > +
> > +	switch(cpu_to_le16(pkt->hdr.op)) {
> 
> I'd expect this to be le16_to_cpu.
> Does this pass check build?

You are right, make C=2 warns about this and it should be le16_to_cpu().
I'll run check builds from now on.

Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)