Re: [QEMU PATCH v4 2/3] virtio-net: introduce a new macaddr control

From: Stefan Hajnoczi <stefanha@gmail.com>
To: akong@redhat.com
Cc: kvm@vger.kernel.org, mst@redhat.com, qemu-devel@nongnu.org,
	virtualization@lists.linux-foundation.org, stefanha@redhat.com
Subject: Re: [QEMU PATCH v4 2/3] virtio-net: introduce a new macaddr control
Date: Mon, 21 Jan 2013 17:08:26 +0100	[thread overview]
Message-ID: <20130121160826.GD24473@stefanha-thinkpad.redhat.com> (raw)
In-Reply-To: <1358560468-10865-3-git-send-email-akong@redhat.com>

On Sat, Jan 19, 2013 at 09:54:27AM +0800, akong@redhat.com wrote:
> @@ -350,6 +351,18 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
>      struct virtio_net_ctrl_mac mac_data;
>      size_t s;
>  
> +    if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
> +        if (iov_size(iov, iov_cnt) != ETH_ALEN) {
> +            return VIRTIO_NET_ERR;
> +        }
> +        s = iov_to_buf(iov, iov_cnt, 0, &n->mac, sizeof(n->mac));
> +        if (s != sizeof(n->mac)) {
> +            return VIRTIO_NET_ERR;
> +        }

Since iov_size() was checked before iov_to_buf(), we never hit this
error.  And if we did n->mac would be trashed (i.e. error handling is
not complete).

I think assert(s == sizeof(n->mac)) is more appropriate appropriate.
Also, please change ETH_ALEN to sizeof(n->mac) to make the relationship
between the check and the copy clear.

Stefan