Re: [Qemu-devel] [PATCH] iscsi: Avoid potential for get_status overflow

[Eric Blake]

On 05/10/2018 08:28 AM, Paolo Bonzini wrote:

On 08/05/2018 23:27, Eric Blake wrote:

Detected by Coverity: Multiplying two 32-bit int and assigning
the result to a 64-bit number is a risk of overflow.  Prior to
the conversion to byte-based interfaces, the block layer took
care of ensuring that a status request never exceeded 2G in
the driver; but after that conversion, the block layer expects
drivers to deal with any size request (the driver can always
truncate the request size back down, as long as it makes
progress).  So, in the off-chance that someone makes a large
request, we are at the mercy of whether iscsi_get_lba_status_task()
will cap things to at most INT_MAX / iscsilun->block_size when
it populates lbasd->num_blocks; since I could not easily audit
that, it's better to be safe than sorry by just forcing a 64-bit
multiply.

Fixes: 92809c36
CC: qemu-sta...@nongnu.org
Signed-off-by: Eric Blake 
---




Queued, thanks.


It's been more than a month since this was queued but it is still not on 
mainline - did it get lost?


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

Re: [Qemu-devel] [PATCH] iscsi: Avoid potential for get_status overflow

[Paolo Bonzini]

On 08/05/2018 23:27, Eric Blake wrote:
> Detected by Coverity: Multiplying two 32-bit int and assigning
> the result to a 64-bit number is a risk of overflow.  Prior to
> the conversion to byte-based interfaces, the block layer took
> care of ensuring that a status request never exceeded 2G in
> the driver; but after that conversion, the block layer expects
> drivers to deal with any size request (the driver can always
> truncate the request size back down, as long as it makes
> progress).  So, in the off-chance that someone makes a large
> request, we are at the mercy of whether iscsi_get_lba_status_task()
> will cap things to at most INT_MAX / iscsilun->block_size when
> it populates lbasd->num_blocks; since I could not easily audit
> that, it's better to be safe than sorry by just forcing a 64-bit
> multiply.
> 
> Fixes: 92809c36
> CC: qemu-sta...@nongnu.org
> Signed-off-by: Eric Blake 
> ---
>  block/iscsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 35423ded03b..a6311b9a320 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -732,7 +732,7 @@ retry:
>  goto out_unlock;
>  }
> 
> -*pnum = lbasd->num_blocks * iscsilun->block_size;
> +*pnum = (int64_t) lbasd->num_blocks * iscsilun->block_size;
> 
>  if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED ||
>  lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) {
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH] iscsi: Avoid potential for get_status overflow

[Philippe Mathieu-Daudé]

On 05/08/2018 06:27 PM, Eric Blake wrote:
> Detected by Coverity: Multiplying two 32-bit int and assigning
> the result to a 64-bit number is a risk of overflow.  Prior to
> the conversion to byte-based interfaces, the block layer took
> care of ensuring that a status request never exceeded 2G in
> the driver; but after that conversion, the block layer expects
> drivers to deal with any size request (the driver can always
> truncate the request size back down, as long as it makes
> progress).  So, in the off-chance that someone makes a large
> request, we are at the mercy of whether iscsi_get_lba_status_task()
> will cap things to at most INT_MAX / iscsilun->block_size when
> it populates lbasd->num_blocks; since I could not easily audit
> that, it's better to be safe than sorry by just forcing a 64-bit
> multiply.

:)

> 
> Fixes: 92809c36
> CC: qemu-sta...@nongnu.org
> Signed-off-by: Eric Blake 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  block/iscsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 35423ded03b..a6311b9a320 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -732,7 +732,7 @@ retry:
>  goto out_unlock;
>  }
> 
> -*pnum = lbasd->num_blocks * iscsilun->block_size;
> +*pnum = (int64_t) lbasd->num_blocks * iscsilun->block_size;
> 
>  if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED ||
>  lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) {
> 



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] iscsi: Avoid potential for get_status overflow

[Eric Blake]

Detected by Coverity: Multiplying two 32-bit int and assigning
the result to a 64-bit number is a risk of overflow.  Prior to
the conversion to byte-based interfaces, the block layer took
care of ensuring that a status request never exceeded 2G in
the driver; but after that conversion, the block layer expects
drivers to deal with any size request (the driver can always
truncate the request size back down, as long as it makes
progress).  So, in the off-chance that someone makes a large
request, we are at the mercy of whether iscsi_get_lba_status_task()
will cap things to at most INT_MAX / iscsilun->block_size when
it populates lbasd->num_blocks; since I could not easily audit
that, it's better to be safe than sorry by just forcing a 64-bit
multiply.

Fixes: 92809c36
CC: qemu-sta...@nongnu.org
Signed-off-by: Eric Blake 
---
 block/iscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 35423ded03b..a6311b9a320 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -732,7 +732,7 @@ retry:
 goto out_unlock;
 }

-*pnum = lbasd->num_blocks * iscsilun->block_size;
+*pnum = (int64_t) lbasd->num_blocks * iscsilun->block_size;

 if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED ||
 lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) {
-- 
2.14.3