| Message ID | 20171206112116.29413-1-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: check current request object before use | expand |
On 06/12/2017 12:21, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > During a dma access, SCSIRequest object 'current_req' could be > null, leading to a null pointer dereference. Add check to avoid > it. > > Reported-by: Zhangboxian <zhangboxian@huawei.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> Sorry, this needs a test case. Paolo > --- > hw/scsi/esp.c | 2 +- > hw/scsi/scsi-bus.c | 3 +++ > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c > index ee586e7d6c..0c6925a708 100644 > --- a/hw/scsi/esp.c > +++ b/hw/scsi/esp.c > @@ -273,7 +273,7 @@ static void esp_do_dma(ESPState *s) > s->ti_size += len; > else > s->ti_size -= len; > - if (s->async_len == 0) { > + if (s->current_req && s->async_len == 0) { > scsi_req_continue(s->current_req); > /* If there is still data to be read from the device then > complete the DMA operation immediately. Otherwise defer > diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c > index 977f7bce1f..ac64a239e9 100644 > --- a/hw/scsi/scsi-bus.c > +++ b/hw/scsi/scsi-bus.c > @@ -1338,6 +1338,9 @@ void scsi_req_unref(SCSIRequest *req) > will start the next chunk or complete the command. */ > void scsi_req_continue(SCSIRequest *req) > { > + if (!req) { > + return; > + } > if (req->io_canceled) { > trace_scsi_req_continue_canceled(req->dev->id, req->lun, req->tag); > return; >
Are both tests for NULL necessary, the second one would seem to
suffice - but also the first check changes whether esp_dma_done()
would get called or not here:
276 if (s->async_len == 0) {
277 scsi_req_continue(s->current_req);
278 /* If there is still data to be read from the device then
279 complete the DMA operation immediately. Otherwise defer
280 until the scsi layer has completed. */
281 if (to_device || s->dma_left != 0 || s->ti_size == 0) {
282 return;
283 }
284 }
285
286 /* Partially filled a scsi buffer. Complete immediately. */
287 esp_dma_done(s);
I don't know the SCSI code well enough to know if it is correct to
change this, so just pointing it out in case someone else might.
Thanks,
Darren.
On Wed, Dec 06, 2017 at 04:51:16PM +0530, P J P wrote:
>From: Prasad J Pandit <pjp@fedoraproject.org>
>
>During a dma access, SCSIRequest object 'current_req' could be
>null, leading to a null pointer dereference. Add check to avoid
>it.
>
>Reported-by: Zhangboxian <zhangboxian@huawei.com>
>Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>---
> hw/scsi/esp.c | 2 +-
> hw/scsi/scsi-bus.c | 3 +++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>index ee586e7d6c..0c6925a708 100644
>--- a/hw/scsi/esp.c
>+++ b/hw/scsi/esp.c
>@@ -273,7 +273,7 @@ static void esp_do_dma(ESPState *s)
> s->ti_size += len;
> else
> s->ti_size -= len;
>- if (s->async_len == 0) {
>+ if (s->current_req && s->async_len == 0) {
> scsi_req_continue(s->current_req);
> /* If there is still data to be read from the device then
> complete the DMA operation immediately. Otherwise defer
>diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
>index 977f7bce1f..ac64a239e9 100644
>--- a/hw/scsi/scsi-bus.c
>+++ b/hw/scsi/scsi-bus.c
>@@ -1338,6 +1338,9 @@ void scsi_req_unref(SCSIRequest *req)
> will start the next chunk or complete the command. */
> void scsi_req_continue(SCSIRequest *req)
> {
>+ if (!req) {
>+ return;
>+ }
> if (req->io_canceled) {
> trace_scsi_req_continue_canceled(req->dev->id, req->lun, req->tag);
> return;
>--
>2.13.6
>
>
On 06/12/2017 12:34, Darren Kenny wrote: > Are both tests for NULL necessary, the second one would seem to > suffice - but also the first check changes whether esp_dma_done() > would get called or not here: > > 276 if (s->async_len == 0) { > 277 scsi_req_continue(s->current_req); > 278 /* If there is still data to be read from the device then > 279 complete the DMA operation immediately. Otherwise defer > 280 until the scsi layer has completed. */ > 281 if (to_device || s->dma_left != 0 || s->ti_size == 0) { > 282 return; > 283 } > 284 } > 285 286 /* Partially filled a scsi buffer. Complete immediately. */ > 287 esp_dma_done(s); > > I don't know the SCSI code well enough to know if it is correct to > change this, so just pointing it out in case someone else might. The second test should definitely not be necessary, it's only papering over a bug elsewhere. But without a testcase it's not possible for me to judge if the first test is the right fix, or _also_ papering over a bug elsewhere. Thanks, Paolo > Thanks, > > Darren. > > On Wed, Dec 06, 2017 at 04:51:16PM +0530, P J P wrote: >> From: Prasad J Pandit <pjp@fedoraproject.org> >> >> During a dma access, SCSIRequest object 'current_req' could be >> null, leading to a null pointer dereference. Add check to avoid >> it. >> >> Reported-by: Zhangboxian <zhangboxian@huawei.com> >> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >> --- >> hw/scsi/esp.c | 2 +- >> hw/scsi/scsi-bus.c | 3 +++ >> 2 files changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c >> index ee586e7d6c..0c6925a708 100644 >> --- a/hw/scsi/esp.c >> +++ b/hw/scsi/esp.c >> @@ -273,7 +273,7 @@ static void esp_do_dma(ESPState *s) >> s->ti_size += len; >> else >> s->ti_size -= len; >> - if (s->async_len == 0) { >> + if (s->current_req && s->async_len == 0) { >> scsi_req_continue(s->current_req); >> /* If there is still data to be read from the device then >> complete the DMA operation immediately. Otherwise defer >> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c >> index 977f7bce1f..ac64a239e9 100644 >> --- a/hw/scsi/scsi-bus.c >> +++ b/hw/scsi/scsi-bus.c >> @@ -1338,6 +1338,9 @@ void scsi_req_unref(SCSIRequest *req) >> will start the next chunk or complete the command. */ >> void scsi_req_continue(SCSIRequest *req) >> { >> + if (!req) { >> + return; >> + } >> if (req->io_canceled) { >> trace_scsi_req_continue_canceled(req->dev->id, req->lun, >> req->tag); >> return; >> -- >> 2.13.6 >> >>
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index ee586e7d6c..0c6925a708 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -273,7 +273,7 @@ static void esp_do_dma(ESPState *s) s->ti_size += len; else s->ti_size -= len; - if (s->async_len == 0) { + if (s->current_req && s->async_len == 0) { scsi_req_continue(s->current_req); /* If there is still data to be read from the device then complete the DMA operation immediately. Otherwise defer diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c index 977f7bce1f..ac64a239e9 100644 --- a/hw/scsi/scsi-bus.c +++ b/hw/scsi/scsi-bus.c @@ -1338,6 +1338,9 @@ void scsi_req_unref(SCSIRequest *req) will start the next chunk or complete the command. */ void scsi_req_continue(SCSIRequest *req) { + if (!req) { + return; + } if (req->io_canceled) { trace_scsi_req_continue_canceled(req->dev->id, req->lun, req->tag); return;