| Message ID | 20231218150213.27253-1-adiupina@astralinux.ru |
|---|---|
| State | New |
| Headers | show |
| Series | esp: process the result of scsi_device_find() | expand |
Cc'ing Mark for the logical change (should we rather assert?). On 18/12/23 16:02, Alexandra Diupina wrote: > Add a 'current_lun' check for a null value > to avoid null pointer dereferencing > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 4eb8606560 (esp: store lun coming from the MESSAGE OUT phase) > Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru> > --- > hw/scsi/esp.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c > index 9b11d8c573..3a2ec35f9b 100644 > --- a/hw/scsi/esp.c > +++ b/hw/scsi/esp.c > @@ -292,6 +292,11 @@ static void do_command_phase(ESPState *s) > esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen); > > current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun); > + > + if (!current_lun) { > + return; > + } > + > s->current_req = scsi_req_new(current_lun, 0, s->lun, buf, cmdlen, s); > datalen = scsi_req_enqueue(s->current_req); > s->ti_size = datalen;
On 26/12/2023 12:08, Philippe Mathieu-Daudé wrote: > Cc'ing Mark for the logical change (should we rather assert?). > > On 18/12/23 16:02, Alexandra Diupina wrote: >> Add a 'current_lun' check for a null value >> to avoid null pointer dereferencing >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Fixes: 4eb8606560 (esp: store lun coming from the MESSAGE OUT phase) >> Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru> >> --- >> hw/scsi/esp.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c >> index 9b11d8c573..3a2ec35f9b 100644 >> --- a/hw/scsi/esp.c >> +++ b/hw/scsi/esp.c >> @@ -292,6 +292,11 @@ static void do_command_phase(ESPState *s) >> esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen); >> current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun); >> + >> + if (!current_lun) { >> + return; >> + } >> + >> s->current_req = scsi_req_new(current_lun, 0, s->lun, buf, cmdlen, s); >> datalen = scsi_req_enqueue(s->current_req); >> s->ti_size = datalen; Checking for the NULL return from scsi_device_find() looks correct to me, but I don't think that a simple return will allow the host to recover. I suspect the right thing to do here is to duplicate the scsi_device_find() logic just above i.e. set the INTR_DC bit and then raise the IRQ if NULL is returned. ATB, Mark.
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 9b11d8c573..3a2ec35f9b 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -292,6 +292,11 @@ static void do_command_phase(ESPState *s) esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen); current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun); + + if (!current_lun) { + return; + } + s->current_req = scsi_req_new(current_lun, 0, s->lun, buf, cmdlen, s); datalen = scsi_req_enqueue(s->current_req); s->ti_size = datalen;
Add a 'current_lun' check for a null value to avoid null pointer dereferencing Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 4eb8606560 (esp: store lun coming from the MESSAGE OUT phase) Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru> --- hw/scsi/esp.c | 5 +++++ 1 file changed, 5 insertions(+)