On 06/20/2013 04:11 PM, Dan Carpenter wrote:
> Hello Alex Elder,
>
> The patch bb23e37acb2a: "rbd: refactor rbd_header_from_disk()" from
> May 6, 2013, has some integer overflow bugs:
>
> drivers/block/rbd.c
> 793 snap_count = le32_to_cpu(ondisk->snap_count);
> 794 snapc = ceph_create_snap_context(snap_count, GFP_KERNEL);
>
> snap_count comes from the disk. On 32 bit systems there is an integer
> overflow problem inside ceph_create_snap_context() so snapc could be
> smaller than intended.
Is it true that fixing ceph_create_snap_context() would resolve
the issues you describe here?
I believe inserting this at the top of that function would
resolve the potential overflow:
if (snap_count > SNAP_COUNT_MAX)
return NULL;
Where SNAP_COUNT_MAX would be defined as:
#define SNAP_COUNT_MAX \
((SIZE_MAX - sizeof (struct ceph_snap_context)) / sizeof (u64))
Do you agree, or is there something else you would recommend?
Thanks.
-Alex
>
> 795 if (!snapc)
> 796 goto out_err;
> 797 snapc->seq = le64_to_cpu(ondisk->snap_seq);
> 798 if (snap_count) {
> 799 struct rbd_image_snap_ondisk *snaps;
> 800 u64 snap_names_len = le64_to_cpu(ondisk->snap_names_len);
> 801
> 802 /* We'll keep a copy of the snapshot names... */
> 803
> 804 if (snap_names_len > (u64)SIZE_MAX)
> 805 goto out_2big;
> 806 snap_names = kmalloc(snap_names_len, GFP_KERNEL);
> 807 if (!snap_names)
> 808 goto out_err;
> 809
> 810 /* ...as well as the array of their sizes. */
> 811
> 812 size = snap_count * sizeof (*header->snap_sizes);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> There is a second integer overflow bug here.
>
> 813 snap_sizes = kmalloc(size, GFP_KERNEL);
> 814 if (!snap_sizes)
> 815 goto out_err;
>
> regards,
> dan carpenter
>
--
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
