On 07/26/2012 12:08 PM, Alex Elder wrote:
This fixes a few issues in rbd_header_from_disk():
- 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.
Why not do this for snap_sizes too?
- Finally, move the definition of the local variable "i" to the
innermost scope in which it's needed.
Signed-off-by: Alex Elder <elder@xxxxxxxxxxx>
Looks good.
Reviewed-by: Josh Durgin <josh.durgin@xxxxxxxxxxx>
---
drivers/block/rbd.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 4584500..3daf8fb 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -494,14 +494,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 (!rbd_dev_ondisk_valid(ondisk))
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),
@@ -509,8 +509,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)
@@ -520,6 +520,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;
}
@@ -544,6 +546,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);
@@ -552,7 +556,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);
}
@@ -562,8 +566,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;
}
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
