[4/4] kvm tool: check the cluster boundary in the qcow read code.

Commit Message

Prasad Joshi April 15, 2011, 2:18 p.m. UTC

Add a new function qcow1_read_cluster() to read a qcow cluster size data at a
time. The function qcow1_read_sector() is modified to use the function
qcow1_read_cluster().

Signed-off-by: Prasad Joshi <[e-mail]>
---
 tools/kvm/qcow.c |  123 ++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 78 insertions(+), 45 deletions(-)

Comments

Pekka Enberg April 16, 2011, 9:23 a.m. UTC | #1

On Fri, Apr 15, 2011 at 5:18 PM, Prasad Joshi <[e-mail]> wrote:
> Add a new function qcow1_read_cluster() to read a qcow cluster size data at a
> time. The function qcow1_read_sector() is modified to use the function
> qcow1_read_cluster().
>
> Signed-off-by: Prasad Joshi <[e-mail]>

I applied patches 1-3 but this patch needs splitting:

  - s/uint64_t/u64/ fixes need to be separate

  - checking for out of bound indices

  - reads that pass cluster bondaries

                        Pekka
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Prasad Joshi April 16, 2011, 11:28 a.m. UTC | #2

On Sat, Apr 16, 2011 at 10:23 AM, Pekka Enberg <[e-mail]> wrote:
> On Fri, Apr 15, 2011 at 5:18 PM, Prasad Joshi <[e-mail]> wrote:
>> Add a new function qcow1_read_cluster() to read a qcow cluster size data at a
>> time. The function qcow1_read_sector() is modified to use the function
>> qcow1_read_cluster().
>>
>> Signed-off-by: Prasad Joshi <[e-mail]>
>
> I applied patches 1-3 but this patch needs splitting:
>
>  - s/uint64_t/u64/ fixes need to be separate

Okay

>
>  - checking for out of bound indices

Only new check added is to verify that the cluster offset is less than
the size of the cluster.

Can this one liner changed be combined with the patch that checks for
cluster boundary?

Checking for cluster boundary adds a new function and the most of the
code from qcow1_read_sector() will be moved to qcow1_read_cluster().

Thanks and Regards,
Prasad
>
>  - reads that pass cluster bondaries
>
>                        Pekka
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/tools/kvm/qcow.c b/tools/kvm/qcow.c
index 9b9af86..e6d5897 100644
--- a/tools/kvm/qcow.c
+++ b/tools/kvm/qcow.c
@@ -36,68 +36,101 @@  static inline uint64_t get_cluster_offset(struct qcow *q, uint64_t offset)
 	return offset & ((1 << header->cluster_bits)-1);
 }
 
-static int qcow1_read_sector(struct disk_image *self, uint64_t sector, void *dst, uint32_t dst_len)
+static uint32_t qcow1_read_cluster(struct qcow *q, uint64_t offset, void *dst, uint32_t dst_len)
 {
-	struct qcow *q = self->priv;
 	struct qcow1_header *header = q->header;
-	uint64_t l2_table_offset;
-	uint64_t l2_table_size;
-	uint64_t clust_offset;
-	uint64_t clust_start;
-	uint64_t *l2_table;
-	uint64_t l1_idx;
-	uint64_t l2_idx;
-	uint64_t offset;
-
-	offset		= sector << SECTOR_SHIFT;
-	if (offset >= header->size)
-		goto out_error;
+	struct qcow_table *table    = &q->table;
+	uint32_t length;
+
+	uint32_t l2_table_size;
+	u64 l2_table_offset;
+	u64 *l2_table;
+	u64 l2_idx;
 
-	l1_idx		= get_l1_index(q, offset);
+	uint32_t clust_size;
+	u64 clust_offset;
+	u64 clust_start;
 
-	if (l1_idx >= q->table.table_size)
-		goto out_error;
+	u64 l1_idx;
 
-	l2_table_offset	= q->table.l1_table[l1_idx];
-	if (!l2_table_offset)
-		goto zero_sector;
+	clust_size    = 1 << header->cluster_bits;
+	l2_table_size = 1 << header->l2_bits;
 
-	l2_table_size	= 1 << header->l2_bits;
+	l1_idx = get_l1_index(q, offset);
+	if (l1_idx >= table->table_size)
+		goto error;
 
-	l2_table	= calloc(l2_table_size, sizeof(uint64_t));
-	if (!l2_table)
-		goto out_error;
+	l2_idx = get_l2_index(q, offset);
+	if (l2_idx >= l2_table_size)
+		goto error;
 
-	if (pread_in_full(q->fd, l2_table, sizeof(uint64_t) * l2_table_size, l2_table_offset) < 0)
-		goto out_error_free_l2;
+	clust_offset = get_cluster_offset(q, offset);
+	if (clust_offset >= clust_size)
+		goto error;
 
-	l2_idx		= get_l2_index(q, offset);
+	length = clust_size - clust_offset;
+	if (length > dst_len)
+		length = dst_len;
 
-	if (l2_idx >= l2_table_size)
-		goto out_error_free_l2;
+	l2_table_offset = table->l1_table[l1_idx];
+	if (!l2_table_offset) {
+		/* unallocated level 2 table: returned zeroed data */
+		memset(dst, 0, length);
+		goto out;
+	}
 
-	clust_start	= be64_to_cpu(l2_table[l2_idx]);
+	l2_table = calloc(l2_table_size, sizeof(u64));
+	if (!l2_table)
+		goto error;
 
-	if (!clust_start)
-		goto zero_sector;
+	if (pread_in_full(q->fd, l2_table, sizeof(u64) * l2_table_size,
+				l2_table_offset) < 0)
+		goto free_l2;
 
-	clust_offset	= get_cluster_offset(q, offset);
+	clust_start = be64_to_cpu(l2_table[l2_idx]);
+	if (!clust_start) {
+		/* unalloacted cluster return zeroed data */
+		memset(dst, 0, length);
+		free(l2_table);
+		goto out;
+	}
 
-	if (pread_in_full(q->fd, dst, dst_len, clust_start + clust_offset) < 0)
-		goto out_error_free_l2;
+	if (pread_in_full(q->fd, dst, length, clust_start + clust_offset) < 0)
+		goto free_l2;
 
+out:
+	return length;
+free_l2:
 	free(l2_table);
-
+error:
 	return 0;
+}
 
-zero_sector:
-	memset(dst, 0, dst_len);
+static int qcow1_read_sector(struct disk_image *self, uint64_t sector, void *dst, uint32_t dst_len)
+{
+	struct qcow *q = self->priv;
+	struct qcow1_header *header = q->header;
+	uint32_t length = 0;
+	char *buf = dst;
+	uint64_t offset;
+	uint32_t nr;
 
-	return 0;
+	while (length < dst_len) {
+		offset = sector << SECTOR_SHIFT;
+		if (offset >= header->size)
+			goto error;
 
-out_error_free_l2:
-	free(l2_table);
-out_error:
+		nr = qcow1_read_cluster(q, offset, buf, dst_len - length);
+		if (!nr)
+			goto error;
+
+		length += nr;
+		buf    += nr;
+		sector += (nr >> SECTOR_SHIFT);
+	}
+
+	return 0;
+error:
 	return -1;
 }