scsi: check current request object before use
PATCH 1/1 scsi: check current request object before use
From: Prasad J Pandit <[email protected]>
During a dma access, SCSIRequest object 'current_req' could be null, leading to a null pointer dereference. Add check to avoid it.
--- hw/scsi/esp.c | 2 +- hw/scsi/scsi-bus.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)
hw/scsi/esp.c +1 -1
@@ -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
hw/scsi/scsi-bus.c +3 -0
@@ -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 ;
Signature · 1 line
2.13.6
Quoted 8 lines · P J P
From: Prasad J Pandit <[email protected]>
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 <[email protected]>Signed-off-by Prasad J Pandit <[email protected]>
Sorry, this needs a test case.
Paolo
Quoted 33 lines
--- hw/scsi/esp.c | 2 +- hw/scsi/scsi-bus.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)hw/scsi/esp.c +1
-1@@ -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 hw/scsi/scsi-bus.c +3
-0@@ -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.
Quoted 44 lines · P J P
From: Prasad J Pandit <[email protected]>
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 <[email protected]>Signed-off-by Prasad J Pandit <[email protected]>---
hw/scsi/esp.c| 2 +-hw/scsi/scsi-bus.c| 3 +++2 files changed, 4 insertions(+), 1 deletion(-)hw/scsi/esp.c +1
-1@@ -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 hw/scsi/scsi-bus.c +3
-0@@ -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 ;Signature · 1 line
2.13.6
Quoted 16 lines · Darren Kenny
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) {
277scsi_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. */ 287esp_dma_done(s);
Quoted 2 lines
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
Quoted 50 lines
Thanks,
Darren.
Quoted 45 lines · P J P
From: Prasad J Pandit <[email protected]>
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 <[email protected]>Signed-off-by Prasad J Pandit <[email protected]>---
hw/scsi/esp.c | 2 +-hw/scsi/scsi-bus.c| 3 +++2 files changed, 4 insertions(+), 1 deletion(-)hw/scsi/esp.c +0
-0@@ -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 deferhw/scsi/scsi-bus.c +0
-0@@ -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