Re: [Qemu-devel] [PATCH v2 3/3] block/iscsi: fix ioctl cancel use-after-free

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3676 bytes --]

On Fri, Feb 09, 2018 at 06:50:06PM +0100, Paolo Bonzini wrote:
> On 03/02/2018 07:16, Stefan Hajnoczi wrote:
> > iscsi_aio_cancel() does not increment the request's reference count,
> > causing a use-after-free when ABORT TASK finishes after the request has
> > already completed.
> > 
> > There are some additional issues with iscsi_aio_cancel():
> > 1. Several ABORT TASKs may be sent for the same task if
> >    iscsi_aio_cancel() is invoked multiple times.  It's better to avoid
> >    this just in case the command identifier is reused.
> > 2. The iscsilun->mutex protection is missing in iscsi_aio_cancel().
> > 
> > Reported-by: Felipe Franciosi <felipe@nutanix.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  block/iscsi.c | 21 ++++++++++++++++++---
> >  1 file changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index 1cfe1c647c..8140baac15 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -119,6 +119,7 @@ typedef struct IscsiAIOCB {
> >  #ifdef __linux__
> >      sg_io_hdr_t *ioh;
> >  #endif
> > +    bool cancelled;
> >  } IscsiAIOCB;
> >  
> >  /* libiscsi uses time_t so its enough to process events every second */
> > @@ -282,6 +283,7 @@ static void iscsi_co_init_iscsitask(IscsiLun *iscsilun, struct IscsiTask *iTask)
> >      };
> >  }
> >  
> > +/* Called (via iscsi_service) with QemuMutex held. */
> >  static void
> >  iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,
> >                      void *private_data)
> > @@ -290,6 +292,7 @@ iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,
> >  
> >      acb->status = -ECANCELED;
> >      iscsi_schedule_bh(acb);
> > +    qemu_aio_unref(acb); /* acquired in iscsi_aio_cancel() */
> >  }
> >  
> >  static void
> > @@ -298,14 +301,25 @@ iscsi_aio_cancel(BlockAIOCB *blockacb)
> >      IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
> >      IscsiLun *iscsilun = acb->iscsilun;
> >  
> > -    if (acb->status != -EINPROGRESS) {
> > +    qemu_mutex_lock(&iscsilun->mutex);
> > +
> > +    /* If it was cancelled or completed already, our work is done here */
> > +    if (acb->cancelled || acb->status != -EINPROGRESS) {
> > +        qemu_mutex_unlock(&iscsilun->mutex);
> >          return;
> >      }
> >  
> > +    acb->cancelled = true;
> > +
> > +    qemu_aio_ref(acb); /* released in iscsi_abort_task_cb() */
> > +
> >      /* send a task mgmt call to the target to cancel the task on the target */
> > -    iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
> > -                                     iscsi_abort_task_cb, acb);
> > +    if (iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,
> > +                                         iscsi_abort_task_cb, acb) < 0) {
> > +        qemu_aio_unref(acb); /* since iscsi_abort_task_cb() won't be called */
> > +    }
> >  
> > +    qemu_mutex_unlock(&iscsilun->mutex);
> >  }
> >  
> >  static const AIOCBInfo iscsi_aiocb_info = {
> > @@ -1000,6 +1014,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
> >      acb->bh          = NULL;
> >      acb->status      = -EINPROGRESS;
> >      acb->ioh         = buf;
> > +    acb->cancelled   = false;
> >  
> >      if (req != SG_IO) {
> >          iscsi_ioctl_handle_emulated(acb, req, buf);
> > 
> 
> BTW, this is okay even without the follow-up, since libiscsi seems not
> to obey its contract...

I'm awaiting feedback from Felipe to see if these fixes plus the
follow-up have solved the issue.  Then I'll send a new revision.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]