Re: Segfault in hw/scsi/scsi-disk.c caused by null pointer

[Peter Maydell]

On Fri, 12 Aug 2022 at 15:41, Denis Krienbühl <denis@href.ch> wrote:
>
> I’m not much further with my segfault, though I now know that the number of 
> detaches likely does not matter and it seems to occur during the attach, not 
> the detach part of the code.
>
> I adapted my change to be a bit more sane - I think it might make sense in 
> general, as something is clearly wrong, the code can be reached somehow and 
> in this case we probably just want to stop, instead of pretending everything 
> is okay.
>
> So the following change also works for us, causing no segfaults:
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index efee6739f9..7273cd6c3d 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -775,6 +775,15 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, 
> uint8_t *outbuf)
>          return -1;
>      }
>
> +    /* Avoid null-pointers leading to segfaults below */
> +    if (!s->version) {
> +        return -1;
> +    }
> +
> +    if (!s->vendor) {
> +        return -1;
> +    }
> +
>      /* PAGE CODE == 0 */
>      buflen = req->cmd.xfer;
>      if (buflen > SCSI_MAX_INQUIRY_LEN) {
>
> I still hope to get some feedback from anyone that is familiar with hw/scsi. 
> Hopefully this reaches someone who can shed some light on this.

As I said previously, this is still absolutely wrong.
If we ever get to this function with either of these
fields being NULL then there has been a serious problem,
probably a memory corruption or use-after-free, or
possibly an attempt to use a partially constructed object.

You need to find out why these fields have ended up NULL,
not just paper over the problem.

You could put in
  assert(s->version);
  assert(s->vendor);

if you like, though that doesn't really gain much over
just segfaulting.

thanks
-- PMM