[PATCH 12/12] rbd: fixes in rbd_header_from_disk()

From: Alex Elder <elder@inktank.com>
To: ceph-devel@vger.kernel.org
Subject: [PATCH 12/12] rbd: fixes in rbd_header_from_disk()
Date: Thu, 19 Jul 2012 16:09:12 -0500	[thread overview]
Message-ID: <50087778.3070304@inktank.com> (raw)
In-Reply-To: <500874F5.4090205@inktank.com>

This fixes a few issues in rbd_header_from_disk():
    - The memcmp() call at the beginning of the function is really
      looking at the "text" field of struct rbd_image_header_ondisk.
      While it does lie at the beginning of the structure, the
      comparison should be done against the field, not the structure
      as a whole.
    - There is a check intended to catch overflow, but it's wrong in
      two ways.
	- First, the type we don't want to overflow is size_t, not
	  unsigned int, and there is now a SIZE_MAX we can use for
	  use with that type.
	- Second, we're allocating the snapshot ids and snapshot
	  image sizes separately (each has type u64; on disk they
          grouped together as a rbd_image_header_ondisk structure).
	  So we can use the size of u64 in this overflow check.
    - If there are no snapshots, then there should be no snapshot
      names.  Enforce this, and issue a warning if we encounter a
      header with no snapshots but a non-zero snap_names_len.
    - When saving the snapshot names into the header, be more direct
      in defining the offset in the on-disk structure from which
      they're being copied by using "snap_count" rather than "i"
      in the array index.
    - If an error occurs, the "snapc" and "snap_names" fields are
      freed at the end of the function.  Make those fields be null
      pointers after they're freed, to be explicit that they are
      no longer valid.
    - Finally, move the definition of the local variable "i" to the
      innermost scope in which it's needed.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 6279186..573202a 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -488,14 +488,14 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 				 struct rbd_image_header_ondisk *ondisk,
 				 u32 allocated_snaps)
 {
-	u32 i, snap_count;
+	u32 snap_count;

-	if (memcmp(ondisk, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT)))
+	if (memcmp(&ondisk->text, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT)))
 		return -ENXIO;

 	snap_count = le32_to_cpu(ondisk->snap_count);
-	if (snap_count > (UINT_MAX - sizeof(struct ceph_snap_context))
-			 / sizeof (*ondisk))
+	if (snap_count > (SIZE_MAX - sizeof(struct ceph_snap_context))
+				 / sizeof (u64))
 		return -EINVAL;
 	header->snapc = kmalloc(sizeof(struct ceph_snap_context) +
 				snap_count * sizeof(u64),
@@ -503,8 +503,8 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 	if (!header->snapc)
 		return -ENOMEM;

-	header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
 	if (snap_count) {
+		header->snap_names_len = le64_to_cpu(ondisk->snap_names_len);
 		header->snap_names = kmalloc(header->snap_names_len,
 					     GFP_KERNEL);
 		if (!header->snap_names)
@@ -514,6 +514,8 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 		if (!header->snap_sizes)
 			goto err_names;
 	} else {
+		WARN_ON(ondisk->snap_names_len);
+		header->snap_names_len = 0;
 		header->snap_names = NULL;
 		header->snap_sizes = NULL;
 	}
@@ -538,6 +540,8 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 	header->total_snaps = snap_count;

 	if (snap_count && allocated_snaps == snap_count) {
+		int i;
+
 		for (i = 0; i < snap_count; i++) {
 			header->snapc->snaps[i] =
 				le64_to_cpu(ondisk->snaps[i].id);
@@ -546,7 +550,7 @@ static int rbd_header_from_disk(struct
rbd_image_header *header,
 		}

 		/* copy snapshot names */
-		memcpy(header->snap_names, &ondisk->snaps[i],
+		memcpy(header->snap_names, &ondisk->snaps[snap_count],
 			header->snap_names_len);
 	}

@@ -556,8 +560,11 @@ err_sizes:
 	kfree(header->snap_sizes);
 err_names:
 	kfree(header->snap_names);
+	header->snap_names = NULL;
 err_snapc:
 	kfree(header->snapc);
+	header->snapc = NULL;
+
 	return -ENOMEM;
 }

-- 
1.7.5.4

