From mboxrd@z Thu Jan  1 00:00:00 1970
From: Pekka Enberg <penberg@kernel.org>
Subject: Re: [PATCH] kvm tool: check the cluster boundary in the qcow read
 code.
Date: Thu, 14 Apr 2011 22:59:43 +0300
Message-ID: <1302811183.15231.49.camel@jaguar>
References: <1302809009-3177-1-git-send-email-prasadjoshi124@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 7bit
Cc: mingo@elte.hu, kvm@vger.kernel.org, asias.hejun@gmail.com,
	gorcunov@gmail.com, levinsasha928@gmail.com, kwolf@redhat.com,
	stefanha@linux.vnet.ibm.com, chaitanyakulkarni15@gmail.com
To: Prasad Joshi <prasadjoshi124@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from filtteri6.pp.htv.fi ([213.243.153.189]:40664 "EHLO
	filtteri6.pp.htv.fi" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1755381Ab1DNT7p (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 14 Apr 2011 15:59:45 -0400
In-Reply-To: <1302809009-3177-1-git-send-email-prasadjoshi124@gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Thu, 2011-04-14 at 20:23 +0100, Prasad Joshi wrote:
> Changed the function names from sect_to_l1_offset(), sect_to_l2_offset() to
> get_l1_index(), get_l2_index() as they return index into their respective table.

This patch does a lot more than what's described above. Please split the
changes in two separate patches.

> +	uint32_t length;
> +	uint32_t tmp;
> +	char *buf = dst;
> +
> +	clust_size = 1 << header->cluster_bits;
> +	length = 0;
> +
> +	while (length < dst_len) {
> +		offset = sector << SECTOR_SHIFT;
> +		if (offset >= header->size)
> +			goto out_error;
> +
> +		l1_idx = get_l1_index(self->priv, offset);
> +		if (l1_idx >= q->table.table_size)
> +			goto out_error;
> +
> +		l2_table_offset	= be64_to_cpu(q->table.l1_table[l1_idx]);
> +		if (!l2_table_offset) {
> +			tmp = clust_size;
> +			memset(buf, 0, tmp);
> +			goto next_cluster;
> +		}
> +
> +		l2_table_size = 1 << header->l2_bits;
> +
> +		l2_table = calloc(l2_table_size, sizeof(uint64_t));
> +		if (!l2_table)
> +			goto out_error;
> +
> +		if (pread_in_full(q->fd, l2_table, sizeof(uint64_t) * l2_table_size, l2_table_offset) < 0)
> +			goto out_error_free_l2;
> +
> +		l2_idx = get_l2_index(self->priv, offset);
> +		if (l2_idx >= l2_table_size)
> +			goto out_error_free_l2;
> +
> +		clust_start = be64_to_cpu(l2_table[l2_idx]);
> +		free(l2_table);
> +		if (!clust_start) {
> +			tmp = clust_size;
> +			memset(buf, 0, tmp);
> +		} else {
> +			clust_offset = sect_to_cluster_offset(self->priv, offset);
> +			tmp = clust_size - clust_offset;
> +
> +			if (pread_in_full(q->fd, buf, tmp, clust_start + clust_offset) < 0)
> +				goto out_error;
> +		}
> +
> +next_cluster:
> +		buf     += tmp;
> +		sector  += (tmp >> SECTOR_SHIFT);
> +		length  += tmp;
> +	}

Well, the loop is not exactly making the code any better. If you're
worried about reads that cross over a single cluster, it's probably
better to rename the current function to qcow1_read_cluster() or
something and use that in a loop that makes sure we never cross over a
cluster.

Btw, I think our ->read_sector and ->write_sector functions need
renaming to ->read_sectors and ->write_sectors with little bit
commentary on what they can expect to receive.

			Pekka