[PATCH] virtio_mem: prevent overflow with subblock size

[PATCH] virtio_mem: prevent overflow with subblock size

[Michael S. Tsirkin]

If subblock size is large (e.g. 1G) 32 bit math involving it
can overflow. Rather than try to catch all instances of that,
let's tweak block size to 64 bit.

It ripples through UAPI which is an ABI change, but it's not too late to
make it, and it will allow supporting >4Gbyte blocks while might
become necessary down the road.

Fixes: 5f1f79bbc9e26 ("virtio-mem: Paravirtualized memory hotplug")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/virtio/virtio_mem.c     | 14 +++++++-------
 include/uapi/linux/virtio_mem.h |  4 ++--
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 2f357142ea5e..7b1bece8a331 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -77,7 +77,7 @@ struct virtio_mem {
 	uint64_t requested_size;
 
 	/* The device block size (for communicating with the device). */
-	uint32_t device_block_size;
+	uint64_t device_block_size;
 	/* The translated node id. NUMA_NO_NODE in case not specified. */
 	int nid;
 	/* Physical start address of the memory region. */
@@ -86,7 +86,7 @@ struct virtio_mem {
 	uint64_t region_size;
 
 	/* The subblock size. */
-	uint32_t subblock_size;
+	uint64_t subblock_size;
 	/* The number of subblocks per memory block. */
 	uint32_t nb_sb_per_mb;
 
@@ -1698,9 +1698,9 @@ static int virtio_mem_init(struct virtio_mem *vm)
 	 * - At least the device block size.
 	 * In the worst case, a single subblock per memory block.
 	 */
-	vm->subblock_size = PAGE_SIZE * 1u << max_t(uint32_t, MAX_ORDER - 1,
-						    pageblock_order);
-	vm->subblock_size = max_t(uint32_t, vm->device_block_size,
+	vm->subblock_size = PAGE_SIZE * 1ul << max_t(uint32_t, MAX_ORDER - 1,
+						     pageblock_order);
+	vm->subblock_size = max_t(uint64_t, vm->device_block_size,
 				  vm->subblock_size);
 	vm->nb_sb_per_mb = memory_block_size_bytes() / vm->subblock_size;
 
@@ -1713,8 +1713,8 @@ static int virtio_mem_init(struct virtio_mem *vm)
 
 	dev_info(&vm->vdev->dev, "start address: 0x%llx", vm->addr);
 	dev_info(&vm->vdev->dev, "region size: 0x%llx", vm->region_size);
-	dev_info(&vm->vdev->dev, "device block size: 0x%x",
-		 vm->device_block_size);
+	dev_info(&vm->vdev->dev, "device block size: 0x%llx",
+		 (unsigned long long)vm->device_block_size);
 	dev_info(&vm->vdev->dev, "memory block size: 0x%lx",
 		 memory_block_size_bytes());
 	dev_info(&vm->vdev->dev, "subblock size: 0x%x",
diff --git a/include/uapi/linux/virtio_mem.h b/include/uapi/linux/virtio_mem.h
index a455c488a995..a9ffe041843c 100644
--- a/include/uapi/linux/virtio_mem.h
+++ b/include/uapi/linux/virtio_mem.h
@@ -185,10 +185,10 @@ struct virtio_mem_resp {
 
 struct virtio_mem_config {
 	/* Block size and alignment. Cannot change. */
-	__u32 block_size;
+	__u64 block_size;
 	/* Valid with VIRTIO_MEM_F_ACPI_PXM. Cannot change. */
 	__u16 node_id;
-	__u16 padding;
+	__u8 padding[6];
 	/* Start address of the memory region. Cannot change. */
 	__u64 addr;
 	/* Region size (maximum). Cannot change. */
-- 
MST

Re: [PATCH] virtio_mem: prevent overflow with subblock size

[David Hildenbrand]

On 08.06.20 08:14, Michael S. Tsirkin wrote:
> If subblock size is large (e.g. 1G) 32 bit math involving it
> can overflow. Rather than try to catch all instances of that,
> let's tweak block size to 64 bit.

I fail to see where we could actually trigger an overflow. The reported
warning looked like a false positive to me.

> 
> It ripples through UAPI which is an ABI change, but it's not too late to
> make it, and it will allow supporting >4Gbyte blocks while might
> become necessary down the road.
> 

This might break cloud-hypervisor, who's already implementing this
protocol upstream (ccing Hui).
https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/vm-virtio/src/mem.rs

(blocks in the gigabyte range were never the original intention of
virtio-mem, but I am not completely opposed to that)

> Fixes: 5f1f79bbc9e26 ("virtio-mem: Paravirtualized memory hotplug")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/virtio/virtio_mem.c     | 14 +++++++-------
>  include/uapi/linux/virtio_mem.h |  4 ++--
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index 2f357142ea5e..7b1bece8a331 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c
> @@ -77,7 +77,7 @@ struct virtio_mem {
>  	uint64_t requested_size;
>  
>  	/* The device block size (for communicating with the device). */
> -	uint32_t device_block_size;
> +	uint64_t device_block_size;
>  	/* The translated node id. NUMA_NO_NODE in case not specified. */
>  	int nid;
>  	/* Physical start address of the memory region. */
> @@ -86,7 +86,7 @@ struct virtio_mem {
>  	uint64_t region_size;
>  
>  	/* The subblock size. */
> -	uint32_t subblock_size;
> +	uint64_t subblock_size;
>  	/* The number of subblocks per memory block. */
>  	uint32_t nb_sb_per_mb;
>  
> @@ -1698,9 +1698,9 @@ static int virtio_mem_init(struct virtio_mem *vm)
>  	 * - At least the device block size.
>  	 * In the worst case, a single subblock per memory block.
>  	 */
> -	vm->subblock_size = PAGE_SIZE * 1u << max_t(uint32_t, MAX_ORDER - 1,
> -						    pageblock_order);
> -	vm->subblock_size = max_t(uint32_t, vm->device_block_size,
> +	vm->subblock_size = PAGE_SIZE * 1ul << max_t(uint32_t, MAX_ORDER - 1,
> +						     pageblock_order);
> +	vm->subblock_size = max_t(uint64_t, vm->device_block_size,
>  				  vm->subblock_size);
>  	vm->nb_sb_per_mb = memory_block_size_bytes() / vm->subblock_size;
>  
> @@ -1713,8 +1713,8 @@ static int virtio_mem_init(struct virtio_mem *vm)
>  
>  	dev_info(&vm->vdev->dev, "start address: 0x%llx", vm->addr);
>  	dev_info(&vm->vdev->dev, "region size: 0x%llx", vm->region_size);
> -	dev_info(&vm->vdev->dev, "device block size: 0x%x",
> -		 vm->device_block_size);
> +	dev_info(&vm->vdev->dev, "device block size: 0x%llx",
> +		 (unsigned long long)vm->device_block_size);
>  	dev_info(&vm->vdev->dev, "memory block size: 0x%lx",
>  		 memory_block_size_bytes());
>  	dev_info(&vm->vdev->dev, "subblock size: 0x%x",
> diff --git a/include/uapi/linux/virtio_mem.h b/include/uapi/linux/virtio_mem.h
> index a455c488a995..a9ffe041843c 100644
> --- a/include/uapi/linux/virtio_mem.h
> +++ b/include/uapi/linux/virtio_mem.h
> @@ -185,10 +185,10 @@ struct virtio_mem_resp {
>  
>  struct virtio_mem_config {
>  	/* Block size and alignment. Cannot change. */
> -	__u32 block_size;
> +	__u64 block_size;
>  	/* Valid with VIRTIO_MEM_F_ACPI_PXM. Cannot change. */
>  	__u16 node_id;
> -	__u16 padding;
> +	__u8 padding[6];
>  	/* Start address of the memory region. Cannot change. */
>  	__u64 addr;
>  	/* Region size (maximum). Cannot change. */
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH] virtio_mem: prevent overflow with subblock size

[Michael S. Tsirkin]

On Mon, Jun 08, 2020 at 08:58:31AM +0200, David Hildenbrand wrote:
> On 08.06.20 08:14, Michael S. Tsirkin wrote:
> > If subblock size is large (e.g. 1G) 32 bit math involving it
> > can overflow. Rather than try to catch all instances of that,
> > let's tweak block size to 64 bit.
> 
> I fail to see where we could actually trigger an overflow. The reported
> warning looked like a false positive to me.


So

    const uint64_t size = count * vm->subblock_size;

is it unreasonable for count to be 4K with subblock_size being 1M?

> > 
> > It ripples through UAPI which is an ABI change, but it's not too late to
> > make it, and it will allow supporting >4Gbyte blocks while might
> > become necessary down the road.
> > 
> 
> This might break cloud-hypervisor, who's already implementing this
> protocol upstream (ccing Hui).
> https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/vm-virtio/src/mem.rs
> 
> (blocks in the gigabyte range were never the original intention of
> virtio-mem, but I am not completely opposed to that)


So in that case, can you code up validation in the probe function?


> > Fixes: 5f1f79bbc9e26 ("virtio-mem: Paravirtualized memory hotplug")
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  drivers/virtio/virtio_mem.c     | 14 +++++++-------
> >  include/uapi/linux/virtio_mem.h |  4 ++--
> >  2 files changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> > index 2f357142ea5e..7b1bece8a331 100644
> > --- a/drivers/virtio/virtio_mem.c
> > +++ b/drivers/virtio/virtio_mem.c
> > @@ -77,7 +77,7 @@ struct virtio_mem {
> >  	uint64_t requested_size;
> >  
> >  	/* The device block size (for communicating with the device). */
> > -	uint32_t device_block_size;
> > +	uint64_t device_block_size;
> >  	/* The translated node id. NUMA_NO_NODE in case not specified. */
> >  	int nid;
> >  	/* Physical start address of the memory region. */
> > @@ -86,7 +86,7 @@ struct virtio_mem {
> >  	uint64_t region_size;
> >  
> >  	/* The subblock size. */
> > -	uint32_t subblock_size;
> > +	uint64_t subblock_size;
> >  	/* The number of subblocks per memory block. */
> >  	uint32_t nb_sb_per_mb;
> >  
> > @@ -1698,9 +1698,9 @@ static int virtio_mem_init(struct virtio_mem *vm)
> >  	 * - At least the device block size.
> >  	 * In the worst case, a single subblock per memory block.
> >  	 */
> > -	vm->subblock_size = PAGE_SIZE * 1u << max_t(uint32_t, MAX_ORDER - 1,
> > -						    pageblock_order);
> > -	vm->subblock_size = max_t(uint32_t, vm->device_block_size,
> > +	vm->subblock_size = PAGE_SIZE * 1ul << max_t(uint32_t, MAX_ORDER - 1,
> > +						     pageblock_order);
> > +	vm->subblock_size = max_t(uint64_t, vm->device_block_size,
> >  				  vm->subblock_size);
> >  	vm->nb_sb_per_mb = memory_block_size_bytes() / vm->subblock_size;
> >  
> > @@ -1713,8 +1713,8 @@ static int virtio_mem_init(struct virtio_mem *vm)
> >  
> >  	dev_info(&vm->vdev->dev, "start address: 0x%llx", vm->addr);
> >  	dev_info(&vm->vdev->dev, "region size: 0x%llx", vm->region_size);
> > -	dev_info(&vm->vdev->dev, "device block size: 0x%x",
> > -		 vm->device_block_size);
> > +	dev_info(&vm->vdev->dev, "device block size: 0x%llx",
> > +		 (unsigned long long)vm->device_block_size);
> >  	dev_info(&vm->vdev->dev, "memory block size: 0x%lx",
> >  		 memory_block_size_bytes());
> >  	dev_info(&vm->vdev->dev, "subblock size: 0x%x",
> > diff --git a/include/uapi/linux/virtio_mem.h b/include/uapi/linux/virtio_mem.h
> > index a455c488a995..a9ffe041843c 100644
> > --- a/include/uapi/linux/virtio_mem.h
> > +++ b/include/uapi/linux/virtio_mem.h
> > @@ -185,10 +185,10 @@ struct virtio_mem_resp {
> >  
> >  struct virtio_mem_config {
> >  	/* Block size and alignment. Cannot change. */
> > -	__u32 block_size;
> > +	__u64 block_size;
> >  	/* Valid with VIRTIO_MEM_F_ACPI_PXM. Cannot change. */
> >  	__u16 node_id;
> > -	__u16 padding;
> > +	__u8 padding[6];
> >  	/* Start address of the memory region. Cannot change. */
> >  	__u64 addr;
> >  	/* Region size (maximum). Cannot change. */
> > 
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb

Re: [PATCH] virtio_mem: prevent overflow with subblock size

[teawater]

> 2020年6月8日 14:58，David Hildenbrand <david@redhat.com> 写道：
> 
> On 08.06.20 08:14, Michael S. Tsirkin wrote:
>> If subblock size is large (e.g. 1G) 32 bit math involving it
>> can overflow. Rather than try to catch all instances of that,
>> let's tweak block size to 64 bit.
> 
> I fail to see where we could actually trigger an overflow. The reported
> warning looked like a false positive to me.
> 
>> 
>> It ripples through UAPI which is an ABI change, but it's not too late to
>> make it, and it will allow supporting >4Gbyte blocks while might
>> become necessary down the road.
>> 
> 
> This might break cloud-hypervisor, who's already implementing this
> protocol upstream (ccing Hui).
> https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/vm-virtio/src/mem.rs
> 
> (blocks in the gigabyte range were never the original intention of
> virtio-mem, but I am not completely opposed to that)

If you think virtio_mem need this patch, I think cloud-hypervisor should follow this update (I will post PR for it).

Best,
Hui

> 
>> Fixes: 5f1f79bbc9e26 ("virtio-mem: Paravirtualized memory hotplug")
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> drivers/virtio/virtio_mem.c     | 14 +++++++-------
>> include/uapi/linux/virtio_mem.h |  4 ++--
>> 2 files changed, 9 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
>> index 2f357142ea5e..7b1bece8a331 100644
>> --- a/drivers/virtio/virtio_mem.c
>> +++ b/drivers/virtio/virtio_mem.c
>> @@ -77,7 +77,7 @@ struct virtio_mem {
>> 	uint64_t requested_size;
>> 
>> 	/* The device block size (for communicating with the device). */
>> -	uint32_t device_block_size;
>> +	uint64_t device_block_size;
>> 	/* The translated node id. NUMA_NO_NODE in case not specified. */
>> 	int nid;
>> 	/* Physical start address of the memory region. */
>> @@ -86,7 +86,7 @@ struct virtio_mem {
>> 	uint64_t region_size;
>> 
>> 	/* The subblock size. */
>> -	uint32_t subblock_size;
>> +	uint64_t subblock_size;
>> 	/* The number of subblocks per memory block. */
>> 	uint32_t nb_sb_per_mb;
>> 
>> @@ -1698,9 +1698,9 @@ static int virtio_mem_init(struct virtio_mem *vm)
>> 	 * - At least the device block size.
>> 	 * In the worst case, a single subblock per memory block.
>> 	 */
>> -	vm->subblock_size = PAGE_SIZE * 1u << max_t(uint32_t, MAX_ORDER - 1,
>> -						    pageblock_order);
>> -	vm->subblock_size = max_t(uint32_t, vm->device_block_size,
>> +	vm->subblock_size = PAGE_SIZE * 1ul << max_t(uint32_t, MAX_ORDER - 1,
>> +						     pageblock_order);
>> +	vm->subblock_size = max_t(uint64_t, vm->device_block_size,
>> 				  vm->subblock_size);
>> 	vm->nb_sb_per_mb = memory_block_size_bytes() / vm->subblock_size;
>> 
>> @@ -1713,8 +1713,8 @@ static int virtio_mem_init(struct virtio_mem *vm)
>> 
>> 	dev_info(&vm->vdev->dev, "start address: 0x%llx", vm->addr);
>> 	dev_info(&vm->vdev->dev, "region size: 0x%llx", vm->region_size);
>> -	dev_info(&vm->vdev->dev, "device block size: 0x%x",
>> -		 vm->device_block_size);
>> +	dev_info(&vm->vdev->dev, "device block size: 0x%llx",
>> +		 (unsigned long long)vm->device_block_size);
>> 	dev_info(&vm->vdev->dev, "memory block size: 0x%lx",
>> 		 memory_block_size_bytes());
>> 	dev_info(&vm->vdev->dev, "subblock size: 0x%x",
>> diff --git a/include/uapi/linux/virtio_mem.h b/include/uapi/linux/virtio_mem.h
>> index a455c488a995..a9ffe041843c 100644
>> --- a/include/uapi/linux/virtio_mem.h
>> +++ b/include/uapi/linux/virtio_mem.h
>> @@ -185,10 +185,10 @@ struct virtio_mem_resp {
>> 
>> struct virtio_mem_config {
>> 	/* Block size and alignment. Cannot change. */
>> -	__u32 block_size;
>> +	__u64 block_size;
>> 	/* Valid with VIRTIO_MEM_F_ACPI_PXM. Cannot change. */
>> 	__u16 node_id;
>> -	__u16 padding;
>> +	__u8 padding[6];
>> 	/* Start address of the memory region. Cannot change. */
>> 	__u64 addr;
>> 	/* Region size (maximum). Cannot change. */
>> 
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb

Re: [PATCH] virtio_mem: prevent overflow with subblock size

[David Hildenbrand]

On 08.06.20 09:08, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2020 at 08:58:31AM +0200, David Hildenbrand wrote:
>> On 08.06.20 08:14, Michael S. Tsirkin wrote:
>>> If subblock size is large (e.g. 1G) 32 bit math involving it
>>> can overflow. Rather than try to catch all instances of that,
>>> let's tweak block size to 64 bit.
>>
>> I fail to see where we could actually trigger an overflow. The reported
>> warning looked like a false positive to me.
> 
> 
> So
> 
>     const uint64_t size = count * vm->subblock_size;
> 
> is it unreasonable for count to be 4K with subblock_size being 1M?

virtio_mem_mb_plug_sb() and friends are only called on subblocks
residing within a single Linux memory block. (currently, 128MB .. 2G on
x86-64). A subblock on x86-64 is currently at least 4MB.

So "count * vm->subblock_size" can currently not exceed the Linux memory
block size (in practice, it is max 128MB).

> 
>>>
>>> It ripples through UAPI which is an ABI change, but it's not too late to
>>> make it, and it will allow supporting >4Gbyte blocks while might
>>> become necessary down the road.
>>>
>>
>> This might break cloud-hypervisor, who's already implementing this
>> protocol upstream (ccing Hui).
>> https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/vm-virtio/src/mem.rs
>>
>> (blocks in the gigabyte range were never the original intention of
>> virtio-mem, but I am not completely opposed to that)
> 
> 
> So in that case, can you code up validation in the probe function?

If we would currently have a "block_size" > Linux memory block size, we
bail out.

virtio_mem_init():

if (vm->device_block_size > memory_block_size_bytes()) {
	dev_err(&vm->vdev->dev,
		"The block size is not supported (too big).\n");
	return -EINVAL;
}

So what's reported can currently not happen. Having that said, changing
"subblock_size" to be an uint64_t is a good cleanup, especially for the
future.




-- 
Thanks,

David / dhildenb

Re: [PATCH] virtio_mem: prevent overflow with subblock size

[David Hildenbrand]

On 08.06.20 09:12, teawater wrote:
> 
> 
>> 2020年6月8日 14:58，David Hildenbrand <david@redhat.com> 写道：
>>
>> On 08.06.20 08:14, Michael S. Tsirkin wrote:
>>> If subblock size is large (e.g. 1G) 32 bit math involving it
>>> can overflow. Rather than try to catch all instances of that,
>>> let's tweak block size to 64 bit.
>>
>> I fail to see where we could actually trigger an overflow. The reported
>> warning looked like a false positive to me.
>>
>>>
>>> It ripples through UAPI which is an ABI change, but it's not too late to
>>> make it, and it will allow supporting >4Gbyte blocks while might
>>> become necessary down the road.
>>>
>>
>> This might break cloud-hypervisor, who's already implementing this
>> protocol upstream (ccing Hui).
>> https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/vm-virtio/src/mem.rs
>>
>> (blocks in the gigabyte range were never the original intention of
>> virtio-mem, but I am not completely opposed to that)
> 
> If you think virtio_mem need this patch, I think cloud-hypervisor should follow this update (I will post PR for it).

Thanks Hui. So we can still do last-minute changes if we all agree it
makes sense.

@MST can you rephrase the patch description to highlight that this is a
preparation for the future only and not actually currently broken?
"virtio-mem: convert device block size into 64bit" ...

With that

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH] virtio_mem: prevent overflow with subblock size

[Michael S. Tsirkin]

On Mon, Jun 08, 2020 at 09:17:45AM +0200, David Hildenbrand wrote:
> On 08.06.20 09:08, Michael S. Tsirkin wrote:
> > On Mon, Jun 08, 2020 at 08:58:31AM +0200, David Hildenbrand wrote:
> >> On 08.06.20 08:14, Michael S. Tsirkin wrote:
> >>> If subblock size is large (e.g. 1G) 32 bit math involving it
> >>> can overflow. Rather than try to catch all instances of that,
> >>> let's tweak block size to 64 bit.
> >>
> >> I fail to see where we could actually trigger an overflow. The reported
> >> warning looked like a false positive to me.
> > 
> > 
> > So
> > 
> >     const uint64_t size = count * vm->subblock_size;
> > 
> > is it unreasonable for count to be 4K with subblock_size being 1M?
> 
> virtio_mem_mb_plug_sb() and friends are only called on subblocks
> residing within a single Linux memory block. (currently, 128MB .. 2G on
> x86-64). A subblock on x86-64 is currently at least 4MB.
> 
> So "count * vm->subblock_size" can currently not exceed the Linux memory
> block size (in practice, it is max 128MB).
> 
> > 
> >>>
> >>> It ripples through UAPI which is an ABI change, but it's not too late to
> >>> make it, and it will allow supporting >4Gbyte blocks while might
> >>> become necessary down the road.
> >>>
> >>
> >> This might break cloud-hypervisor, who's already implementing this
> >> protocol upstream (ccing Hui).
> >> https://github.com/cloud-hypervisor/cloud-hypervisor/blob/master/vm-virtio/src/mem.rs
> >>
> >> (blocks in the gigabyte range were never the original intention of
> >> virtio-mem, but I am not completely opposed to that)
> > 
> > 
> > So in that case, can you code up validation in the probe function?
> 
> If we would currently have a "block_size" > Linux memory block size, we
> bail out.
> 
> virtio_mem_init():
> 
> if (vm->device_block_size > memory_block_size_bytes()) {
> 	dev_err(&vm->vdev->dev,
> 		"The block size is not supported (too big).\n");
> 	return -EINVAL;
> }

Sounds good.

> So what's reported can currently not happen. Having that said, changing
> "subblock_size" to be an uint64_t is a good cleanup, especially for the
> future.

OK, no need to argue about it then. I tweaked the subject as you
suggested and queued it then.

> 
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb

Re: [PATCH] virtio_mem: prevent overflow with subblock size

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 9633 bytes --]

Hi "Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on vhost/vhost]
[also build test WARNING on next-20200608]
[cannot apply to linus/master linux/master v5.7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Michael-S-Tsirkin/virtio_mem-prevent-overflow-with-subblock-size/20200608-141805
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project e429cffd4f228f70c1d9df0e5d77c08590dd9766)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/virtio/virtio_mem.c:1721:4: warning: format specifies type 'unsigned int' but the argument has type 'uint64_t' (aka 'unsigned long long') [-Wformat]
vm->subblock_size);
^~~~~~~~~~~~~~~~~
include/linux/dev_printk.h:110:33: note: expanded from macro 'dev_info'
_dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
~~~     ^~~~~~~~~~~
1 warning generated.

vim +1721 drivers/virtio/virtio_mem.c

5f1f79bbc9e26f David Hildenbrand  2020-05-07  1642  
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1643  static int virtio_mem_init(struct virtio_mem *vm)
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1644  {
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1645  	const uint64_t phys_limit = 1UL << MAX_PHYSMEM_BITS;
f2af6d3978d74a David Hildenbrand  2020-05-07  1646  	uint16_t node_id;
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1647  
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1648  	if (!vm->vdev->config->get) {
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1649  		dev_err(&vm->vdev->dev, "config access disabled\n");
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1650  		return -EINVAL;
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1651  	}
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1652  
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1653  	/*
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1654  	 * We don't want to (un)plug or reuse any memory when in kdump. The
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1655  	 * memory is still accessible (but not mapped).
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1656  	 */
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1657  	if (is_kdump_kernel()) {
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1658  		dev_warn(&vm->vdev->dev, "disabled in kdump kernel\n");
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1659  		return -EBUSY;
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1660  	}
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1661  
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1662  	/* Fetch all properties that can't change. */
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1663  	virtio_cread(vm->vdev, struct virtio_mem_config, plugged_size,
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1664  		     &vm->plugged_size);
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1665  	virtio_cread(vm->vdev, struct virtio_mem_config, block_size,
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1666  		     &vm->device_block_size);
f2af6d3978d74a David Hildenbrand  2020-05-07  1667  	virtio_cread(vm->vdev, struct virtio_mem_config, node_id,
f2af6d3978d74a David Hildenbrand  2020-05-07  1668  		     &node_id);
f2af6d3978d74a David Hildenbrand  2020-05-07  1669  	vm->nid = virtio_mem_translate_node_id(vm, node_id);
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1670  	virtio_cread(vm->vdev, struct virtio_mem_config, addr, &vm->addr);
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1671  	virtio_cread(vm->vdev, struct virtio_mem_config, region_size,
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1672  		     &vm->region_size);
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1673  
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1674  	/*
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1675  	 * We always hotplug memory in memory block granularity. This way,
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1676  	 * we have to wait for exactly one memory block to online.
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1677  	 */
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1678  	if (vm->device_block_size > memory_block_size_bytes()) {
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1679  		dev_err(&vm->vdev->dev,
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1680  			"The block size is not supported (too big).\n");
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1681  		return -EINVAL;
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1682  	}
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1683  
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1684  	/* bad device setup - warn only */
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1685  	if (!IS_ALIGNED(vm->addr, memory_block_size_bytes()))
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1686  		dev_warn(&vm->vdev->dev,
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1687  			 "The alignment of the physical start address can make some memory unusable.\n");
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1688  	if (!IS_ALIGNED(vm->addr + vm->region_size, memory_block_size_bytes()))
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1689  		dev_warn(&vm->vdev->dev,
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1690  			 "The alignment of the physical end address can make some memory unusable.\n");
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1691  	if (vm->addr + vm->region_size > phys_limit)
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1692  		dev_warn(&vm->vdev->dev,
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1693  			 "Some memory is not addressable. This can make some memory unusable.\n");
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1694  
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1695  	/*
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1696  	 * Calculate the subblock size:
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1697  	 * - At least MAX_ORDER - 1 / pageblock_order.
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1698  	 * - At least the device block size.
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1699  	 * In the worst case, a single subblock per memory block.
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1700  	 */
09760a643f90c4 Michael S. Tsirkin 2020-06-08  1701  	vm->subblock_size = PAGE_SIZE * 1ul << max_t(uint32_t, MAX_ORDER - 1,
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1702  						     pageblock_order);
09760a643f90c4 Michael S. Tsirkin 2020-06-08  1703  	vm->subblock_size = max_t(uint64_t, vm->device_block_size,
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1704  				  vm->subblock_size);
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1705  	vm->nb_sb_per_mb = memory_block_size_bytes() / vm->subblock_size;
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1706  
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1707  	/* Round up to the next full memory block */
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1708  	vm->first_mb_id = virtio_mem_phys_to_mb_id(vm->addr - 1 +
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1709  						   memory_block_size_bytes());
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1710  	vm->next_mb_id = vm->first_mb_id;
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1711  	vm->last_mb_id = virtio_mem_phys_to_mb_id(vm->addr +
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1712  			 vm->region_size) - 1;
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1713  
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1714  	dev_info(&vm->vdev->dev, "start address: 0x%llx", vm->addr);
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1715  	dev_info(&vm->vdev->dev, "region size: 0x%llx", vm->region_size);
09760a643f90c4 Michael S. Tsirkin 2020-06-08  1716  	dev_info(&vm->vdev->dev, "device block size: 0x%llx",
09760a643f90c4 Michael S. Tsirkin 2020-06-08  1717  		 (unsigned long long)vm->device_block_size);
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1718  	dev_info(&vm->vdev->dev, "memory block size: 0x%lx",
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1719  		 memory_block_size_bytes());
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1720  	dev_info(&vm->vdev->dev, "subblock size: 0x%x",
5f1f79bbc9e26f David Hildenbrand  2020-05-07 @1721  		 vm->subblock_size);
f2af6d3978d74a David Hildenbrand  2020-05-07  1722  	if (vm->nid != NUMA_NO_NODE)
f2af6d3978d74a David Hildenbrand  2020-05-07  1723  		dev_info(&vm->vdev->dev, "nid: %d", vm->nid);
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1724  
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1725  	return 0;
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1726  }
5f1f79bbc9e26f David Hildenbrand  2020-05-07  1727  

:::::: The code at line 1721 was first introduced by commit
:::::: 5f1f79bbc9e26fa9412fa9522f957bb8f030c442 virtio-mem: Paravirtualized memory hotplug

:::::: TO: David Hildenbrand <david@redhat.com>
:::::: CC: Michael S. Tsirkin <mst@redhat.com>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 73449 bytes --]