[PATCH] rbd: fix integer overflow in rbd_header_from_disk()

[PATCH] rbd: fix integer overflow in rbd_header_from_disk()

[Xi Wang]

ondisk->snap_count is read from disk via rbd_req_sync_read() and thus
needs validation.  Otherwise, a bogus `snap_count' could overflow the
kmalloc() size, leading to memory corruption.

Also use `u32' consistently for `snap_count'.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 drivers/block/rbd.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 013c7a5..d47f7e6 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -487,18 +487,20 @@ static void rbd_coll_release(struct kref *kref)
  */
 static int rbd_header_from_disk(struct rbd_image_header *header,
 				 struct rbd_image_header_ondisk *ondisk,
-				 int allocated_snaps,
+				 u32 allocated_snaps,
 				 gfp_t gfp_flags)
 {
-	int i;
-	u32 snap_count;
+	u32 i, snap_count;
 
 	if (memcmp(ondisk, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT)))
 		return -ENXIO;
 
 	snap_count = le32_to_cpu(ondisk->snap_count);
+	if (snap_count > (ULONG_MAX - sizeof(struct ceph_snap_context))
+			 / sizeof(*ondisk))
+		return -EINVAL;
 	header->snapc = kmalloc(sizeof(struct ceph_snap_context) +
-				snap_count * sizeof (*ondisk),
+				snap_count * sizeof(*ondisk),
 				gfp_flags);
 	if (!header->snapc)
 		return -ENOMEM;
@@ -1592,7 +1594,7 @@ static int rbd_read_header(struct rbd_device *rbd_dev,
 {
 	ssize_t rc;
 	struct rbd_image_header_ondisk *dh;
-	int snap_count = 0;
+	u32 snap_count = 0;
 	u64 ver;
 	size_t len;
 
-- 
1.7.5.4

Re: [PATCH] rbd: fix integer overflow in rbd_header_from_disk()

[Alex Elder]

On 04/09/2012 04:52 PM, Xi Wang wrote:
> ondisk->snap_count is read from disk via rbd_req_sync_read() and thus
> needs validation.  Otherwise, a bogus `snap_count' could overflow the
> kmalloc() size, leading to memory corruption.
>
> Also use `u32' consistently for `snap_count'.

This looks good, however I have changed it to use UINT_MAX
rather than ULONG_MAX, because on some architectures size_t
(__kernel_size_t) is defined as type (unsigned int).  It
is the more conservative value, and even on architectures
where __BITS_PER_LONG is 64, it still offers a sane upper
bound on the number of snapshots for a rbd device.

Reviewed-by: Alex Elder <elder@dreamhost.com>

> Signed-off-by: Xi Wang<xi.wang@gmail.com>
> ---
>   drivers/block/rbd.c |   12 +++++++-----
>   1 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 013c7a5..d47f7e6 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -487,18 +487,20 @@ static void rbd_coll_release(struct kref *kref)
>    */
>   static int rbd_header_from_disk(struct rbd_image_header *header,
>   				 struct rbd_image_header_ondisk *ondisk,
> -				 int allocated_snaps,
> +				 u32 allocated_snaps,
>   				 gfp_t gfp_flags)
>   {
> -	int i;
> -	u32 snap_count;
> +	u32 i, snap_count;
>
>   	if (memcmp(ondisk, RBD_HEADER_TEXT, sizeof(RBD_HEADER_TEXT)))
>   		return -ENXIO;
>
>   	snap_count = le32_to_cpu(ondisk->snap_count);
> +	if (snap_count>  (ULONG_MAX - sizeof(struct ceph_snap_context))
> +			 / sizeof(*ondisk))
> +		return -EINVAL;
>   	header->snapc = kmalloc(sizeof(struct ceph_snap_context) +
> -				snap_count * sizeof (*ondisk),
> +				snap_count * sizeof(*ondisk),
>   				gfp_flags);
>   	if (!header->snapc)
>   		return -ENOMEM;
> @@ -1592,7 +1594,7 @@ static int rbd_read_header(struct rbd_device *rbd_dev,
>   {
>   	ssize_t rc;
>   	struct rbd_image_header_ondisk *dh;
> -	int snap_count = 0;
> +	u32 snap_count = 0;
>   	u64 ver;
>   	size_t len;
>

Re: [PATCH] rbd: fix integer overflow in rbd_header_from_disk()

[Xi Wang]

On Apr 18, 2012, at 10:21 AM, Alex Elder wrote:
> 
> This looks good, however I have changed it to use UINT_MAX
> rather than ULONG_MAX, because on some architectures size_t
> (__kernel_size_t) is defined as type (unsigned int).  It
> is the more conservative value, and even on architectures
> where __BITS_PER_LONG is 64, it still offers a sane upper
> bound on the number of snapshots for a rbd device.

Looks good to me.

BTW, on which arch is size_t 32-bit while long is 64?
Several commits still use ULONG_MAX, such as

http://git.kernel.org/linus/64486697

- xi

Re: [PATCH] rbd: fix integer overflow in rbd_header_from_disk()

[Alex Elder]

On 04/18/2012 01:09 PM, Xi Wang wrote:
> On Apr 18, 2012, at 10:21 AM, Alex Elder wrote:
>>
>> This looks good, however I have changed it to use UINT_MAX
>> rather than ULONG_MAX, because on some architectures size_t
>> (__kernel_size_t) is defined as type (unsigned int).  It
>> is the more conservative value, and even on architectures
>> where __BITS_PER_LONG is 64, it still offers a sane upper
>> bound on the number of snapshots for a rbd device.
>
> Looks good to me.
>
> BTW, on which arch is size_t 32-bit while long is 64?
> Several commits still use ULONG_MAX, such as

In my tree, these at least *can* be:  arm, m68k, mips,
powerpc, sparc, x86, and others (including generic).

It may not matter in practice, but I prefer to be very
careful.  As long as you're going out of your way to
avoid overflow might as well make sure it works on all
architectures in the process.

					-Alex

> http://git.kernel.org/linus/64486697
>
> - xi
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] rbd: fix integer overflow in rbd_header_from_disk()

[Xi Wang]

On Apr 18, 2012, at 2:47 PM, Alex Elder wrote:

> It may not matter in practice, but I prefer to be very
> careful.  As long as you're going out of your way to
> avoid overflow might as well make sure it works on all
> architectures in the process.

I agree.  Probably a cleaner way is to define SIZE_MAX
or use KMALLOC_MAX_SIZE.

- xi