[Qemu-devel] [PATCH] vmdk: return ENOTSUP before offset overflow

[Qemu-devel] [PATCH] vmdk: return ENOTSUP before offset overflow

[yuchenlin]

From: yuchenlin <yuchenlin@synology.com>

VMDK has a hard limitation of extent size, which is due to the size of grain
table entry is 32 bits. It means it can only point to a grain located at
offset = 2^32. To prevent offset overflow and record a useless offset
in grain table. We should return un-support here.

Signed-off-by: yuchenlin <yuchenlin@synology.com>
---
 block/vmdk.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index f94c49a9c0..d8fc961940 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -47,6 +47,9 @@
 #define VMDK4_FLAG_MARKER (1 << 17)
 #define VMDK4_GD_AT_END 0xffffffffffffffffULL
 
+/* 2TB */
+#define VMDK_EXTENT_SIZE_LIMIT (2199023255552)
+
 #define VMDK_GTE_ZEROED 0x1
 
 /* VMDK internal error codes */
@@ -1645,6 +1648,9 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
                 return ret;
             }
             if (m_data.valid) {
+                if (cluster_offset > VMDK_EXTENT_SIZE_LIMIT) {
+                    return -ENOTSUP;
+                }
                 /* update L2 tables */
                 if (vmdk_L2update(extent, &m_data,
                                   cluster_offset >> BDRV_SECTOR_BITS)
-- 
2.16.2

Re: [Qemu-devel] [PATCH] vmdk: return ENOTSUP before offset overflow

[Fam Zheng]

On Thu, 03/22 10:40, yuchenlin@synology.com wrote:
> From: yuchenlin <yuchenlin@synology.com>
> 
> VMDK has a hard limitation of extent size, which is due to the size of grain
> table entry is 32 bits. It means it can only point to a grain located at
> offset = 2^32. To prevent offset overflow and record a useless offset
> in grain table. We should return un-support here.
> 
> Signed-off-by: yuchenlin <yuchenlin@synology.com>
> ---
>  block/vmdk.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index f94c49a9c0..d8fc961940 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -47,6 +47,9 @@
>  #define VMDK4_FLAG_MARKER (1 << 17)
>  #define VMDK4_GD_AT_END 0xffffffffffffffffULL
>  
> +/* 2TB */
> +#define VMDK_EXTENT_SIZE_LIMIT (2199023255552)
> +
>  #define VMDK_GTE_ZEROED 0x1
>  
>  /* VMDK internal error codes */
> @@ -1645,6 +1648,9 @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,
>                  return ret;
>              }
>              if (m_data.valid) {
> +                if (cluster_offset > VMDK_EXTENT_SIZE_LIMIT) {

I think this should be >=?

Also the check should be done earlier, in get_cluster_offset even before
m_data.valid is set. We can peek at extent->next_cluster_sector to tell if the
allocation will succeed or not, and avoid writing the user data.

Fam

> +                    return -ENOTSUP;
> +                }
>                  /* update L2 tables */
>                  if (vmdk_L2update(extent, &m_data,
>                                    cluster_offset >> BDRV_SECTOR_BITS)
> -- 
> 2.16.2
> 

Re: [Qemu-devel] [PATCH] vmdk: return ENOTSUP before offset overflow

[Eric Blake]

On 03/21/2018 09:40 PM, yuchenlin--- via Qemu-devel wrote:
> From: yuchenlin <yuchenlin@synology.com>
> 
> VMDK has a hard limitation of extent size, which is due to the size of grain
> table entry is 32 bits. It means it can only point to a grain located at
> offset = 2^32. To prevent offset overflow and record a useless offset
> in grain table. We should return un-support here.
> 
> Signed-off-by: yuchenlin <yuchenlin@synology.com>
> ---
>   block/vmdk.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index f94c49a9c0..d8fc961940 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -47,6 +47,9 @@
>   #define VMDK4_FLAG_MARKER (1 << 17)
>   #define VMDK4_GD_AT_END 0xffffffffffffffffULL
>   
> +/* 2TB */
> +#define VMDK_EXTENT_SIZE_LIMIT (2199023255552)

Please spell this '(2ULL * 1024 * 1024 * 1024 * 1024)', or even rebase 
it on top of Phillipe's BYTE-based definitions as '2 * T_BYTE' (v2 
proposed a cunits.h, although v3 is still pending posting and may rename 
the header units.h)

https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg01077.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org