On Mon, Jan 21, 2013 at 05:08:26PM +0100, Stefan Hajnoczi wrote:
> On Sat, Jan 19, 2013 at 09:54:27AM +0800, ak...@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).
You are right.
iov_size() computes the size by accounting iov[].iov_lens, the first
check is enough.
> 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.
>
Will update this patch.
if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
return VIRTIO_NET_ERR;
}
s = iov_to_buf(iov, iov_cnt, 0, &n->mac, sizeof(n->mac));
assert(s == sizeof(n->mac));
qemu_format_nic_info_str(&n->nic->nc, n->mac);
return VIRTIO_NET_OK;
}
> Stefan
