[PATCH] iscsi: Don't access non-existent scsi_lba_status_descriptor

[PATCH] iscsi: Don't access non-existent scsi_lba_status_descriptor

[Kevin Wolf]

In iscsi_co_block_status(), we may have received num_descriptors == 0
from the iscsi server. Therefore, we can't unconditionally access
lbas->descriptors[0]. Add the missing check.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/iscsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index cbd57294ab..c8feaa2f0e 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -753,7 +753,7 @@ retry:
     }
 
     lbas = scsi_datain_unmarshall(iTask.task);
-    if (lbas == NULL) {
+    if (lbas == NULL || lbas->num_descriptors == 0) {
         ret = -EIO;
         goto out_unlock;
     }
-- 
2.20.1

Re: [PATCH] iscsi: Don't access non-existent scsi_lba_status_descriptor

[Felipe Franciosi]

> On Jan 23, 2020, at 5:05 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> 
> In iscsi_co_block_status(), we may have received num_descriptors == 0
> from the iscsi server. Therefore, we can't unconditionally access
> lbas->descriptors[0]. Add the missing check.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/iscsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index cbd57294ab..c8feaa2f0e 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -753,7 +753,7 @@ retry:
>     }
> 
>     lbas = scsi_datain_unmarshall(iTask.task);
> -    if (lbas == NULL) {
> +    if (lbas == NULL || lbas->num_descriptors == 0) {
>         ret = -EIO;
>         goto out_unlock;
>     }
> -- 
> 2.20.1
> 

Reviewed-by: Felipe Franciosi <felipe@nutanix.com>

Re: [PATCH] iscsi: Don't access non-existent scsi_lba_status_descriptor

[John Snow]

On 1/23/20 12:05 PM, Kevin Wolf wrote:
> In iscsi_co_block_status(), we may have received num_descriptors == 0
> from the iscsi server. Therefore, we can't unconditionally access
> lbas->descriptors[0]. Add the missing check.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/iscsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index cbd57294ab..c8feaa2f0e 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -753,7 +753,7 @@ retry:
>      }
>  
>      lbas = scsi_datain_unmarshall(iTask.task);
> -    if (lbas == NULL) {
> +    if (lbas == NULL || lbas->num_descriptors == 0) {
>          ret = -EIO;
>          goto out_unlock;
>      }
> 

Naive question: Does the specification allow for such a response? Is
this inherently an error?

Anyway, this is better than accessing junk memory, so:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH] iscsi: Don't access non-existent scsi_lba_status_descriptor

[Felipe Franciosi]

> On Jan 23, 2020, at 8:37 PM, John Snow <jsnow@redhat.com> wrote:
> 
> 
> 
> On 1/23/20 12:05 PM, Kevin Wolf wrote:
>> In iscsi_co_block_status(), we may have received num_descriptors == 0
>> from the iscsi server. Therefore, we can't unconditionally access
>> lbas->descriptors[0]. Add the missing check.
>> 
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>> block/iscsi.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index cbd57294ab..c8feaa2f0e 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -753,7 +753,7 @@ retry:
>>     }
>> 
>>     lbas = scsi_datain_unmarshall(iTask.task);
>> -    if (lbas == NULL) {
>> +    if (lbas == NULL || lbas->num_descriptors == 0) {
>>         ret = -EIO;
>>         goto out_unlock;
>>     }
>> 
> 
> Naive question: Does the specification allow for such a response? Is
> this inherently an error?

The spec doesn't say, but libiscsi (which Qemu should trust) may
return zero for num_descriptors with certain server responses (which
no one should trust).

https://github.com/sahlberg/libiscsi/blob/master/lib/scsi-lowlevel.c#L845

F.

> 
> Anyway, this is better than accessing junk memory, so:
> 
> Reviewed-by: John Snow <jsnow@redhat.com>

Re: [PATCH] iscsi: Don't access non-existent scsi_lba_status_descriptor

[Philippe Mathieu-Daudé]

On 1/23/20 6:05 PM, Kevin Wolf wrote:
> In iscsi_co_block_status(), we may have received num_descriptors == 0
> from the iscsi server. Therefore, we can't unconditionally access
> lbas->descriptors[0]. Add the missing check.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/iscsi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index cbd57294ab..c8feaa2f0e 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -753,7 +753,7 @@ retry:
>       }
>   
>       lbas = scsi_datain_unmarshall(iTask.task);
> -    if (lbas == NULL) {
> +    if (lbas == NULL || lbas->num_descriptors == 0) {
>           ret = -EIO;
>           goto out_unlock;
>       }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH] iscsi: Don't access non-existent scsi_lba_status_descriptor

[Peter Lieven]

Am 23.01.2020 um 21:38 schrieb John Snow <jsnow@redhat.com>:
> 
> 
> 
>> On 1/23/20 12:05 PM, Kevin Wolf wrote:
>> In iscsi_co_block_status(), we may have received num_descriptors == 0
>> from the iscsi server. Therefore, we can't unconditionally access
>> lbas->descriptors[0]. Add the missing check.
>> 
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>> block/iscsi.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index cbd57294ab..c8feaa2f0e 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -753,7 +753,7 @@ retry:
>>     }
>> 
>>     lbas = scsi_datain_unmarshall(iTask.task);
>> -    if (lbas == NULL) {
>> +    if (lbas == NULL || lbas->num_descriptors == 0) {
>>         ret = -EIO;
>>         goto out_unlock;
>>     }
>> 
> 
> Naive question: Does the specification allow for such a response? Is
> this inherently an error?

The spec says the answer SHALL contain at least one lbasd. So I think threating zero as an error is okay

Anyway,

Reviewed-by: Peter Lieven <pl@kamp.de>

Peter

Re: [PATCH] iscsi: Don't access non-existent scsi_lba_status_descriptor

[Kevin Wolf]

Am 23.01.2020 um 21:37 hat John Snow geschrieben:
> On 1/23/20 12:05 PM, Kevin Wolf wrote:
> > In iscsi_co_block_status(), we may have received num_descriptors == 0
> > from the iscsi server. Therefore, we can't unconditionally access
> > lbas->descriptors[0]. Add the missing check.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/iscsi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index cbd57294ab..c8feaa2f0e 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -753,7 +753,7 @@ retry:
> >      }
> >  
> >      lbas = scsi_datain_unmarshall(iTask.task);
> > -    if (lbas == NULL) {
> > +    if (lbas == NULL || lbas->num_descriptors == 0) {
> >          ret = -EIO;
> >          goto out_unlock;
> >      }
> > 
> 
> Naive question: Does the specification allow for such a response? Is
> this inherently an error?

Even if iscsi allowed it, it would be a useless response, because it
means that you didn't get the block status of any block.

bdrv_co_block_status() may only return *pnum == 0 at EOF, so I don't
think we have any other option than returning an error. (We could retry,
but if a target returns a useless response once, why should we trust it
do behave better the second time?)

> Anyway, this is better than accessing junk memory, so:
> 
> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks!

Kevin