| Message ID | 20180508212718.1482663-1-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iscsi: Avoid potential for get_status overflow | expand |
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-stable@nongnu.org > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > 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) { >
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-stable@nongnu.org > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > 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
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-stable@nongnu.org >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- > > Queued, thanks. It's been more than a month since this was queued but it is still not on mainline - did it get lost?
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) {
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-stable@nongnu.org Signed-off-by: Eric Blake <eblake@redhat.com> --- block/iscsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)