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 <p...@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 <zhangbox...@huawei.com>
>> Signed-off-by: Prasad J Pandit <p...@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
>>
>>
