| Message ID | 20200123170544.30117-1-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iscsi: Don't access non-existent scsi_lba_status_descriptor | expand |
> 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>
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>
> 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>
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>
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
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
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; }
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(-)