* [Qemu-devel] [PATCH] scsi: check current request object before use
@ 2017-12-06 11:21 P J P
2017-12-06 11:25 ` Paolo Bonzini
2017-12-06 11:34 ` Darren Kenny
0 siblings, 2 replies; 4+ messages in thread
From: P J P @ 2017-12-06 11:21 UTC (permalink / raw)
To: Qemu Developers; +Cc: Paolo Bonzini, Zhangboxian, Prasad J Pandit
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] scsi: check current request object before use
2017-12-06 11:21 [Qemu-devel] [PATCH] scsi: check current request object before use P J P
@ 2017-12-06 11:25 ` Paolo Bonzini
2017-12-06 11:34 ` Darren Kenny
1 sibling, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2017-12-06 11:25 UTC (permalink / raw)
To: P J P, Qemu Developers; +Cc: Zhangboxian, Prasad J Pandit
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;
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] scsi: check current request object before use
2017-12-06 11:21 [Qemu-devel] [PATCH] scsi: check current request object before use P J P
2017-12-06 11:25 ` Paolo Bonzini
@ 2017-12-06 11:34 ` Darren Kenny
2017-12-11 15:56 ` Paolo Bonzini
1 sibling, 1 reply; 4+ messages in thread
From: Darren Kenny @ 2017-12-06 11:34 UTC (permalink / raw)
To: P J P; +Cc: Qemu Developers, Paolo Bonzini, Zhangboxian, Prasad J Pandit
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] scsi: check current request object before use
2017-12-06 11:34 ` Darren Kenny
@ 2017-12-11 15:56 ` Paolo Bonzini
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2017-12-11 15:56 UTC (permalink / raw)
To: P J P, Qemu Developers, Zhangboxian, Prasad J Pandit
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
>>
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-12-11 15:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-06 11:21 [Qemu-devel] [PATCH] scsi: check current request object before use P J P
2017-12-06 11:25 ` Paolo Bonzini
2017-12-06 11:34 ` Darren Kenny
2017-12-11 15:56 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).