MIME-Version: 1.0
References: <20221227001001.51626-1-zh.nvgt@gmail.com>
In-Reply-To: <20221227001001.51626-1-zh.nvgt@gmail.com>
From:   Hang Zhang <zh.nvgt@gmail.com>
Date:   Thu, 2 Feb 2023 22:31:22 -0500
Message-ID: <CAO2zrtbhnry5k+daY0DS0vfNqQM5ajz4hLAmne_dJRa6m4W8xA@mail.gmail.com>
Subject: Re: [PATCH] scsi: dpt_i2o: fix a potential use-after-free in __adpt_reset()
Cc:     Adaptec OEM Raid Solutions <aacraid@microsemi.com>,
        "James E . J . Bottomley" <jejb@linux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
To:     unlisted-recipients:; (no To-header on input)
Precedence: bulk

On Mon, Dec 26, 2022 at 7:10 PM Hang Zhang <zh.nvgt@gmail.com> wrote:
>
> __adpt_reset() invokes adpt_hba_reset(), which can free "pHba"
> on error paths and return an negative error code in those
> situations. The problem is that "pHba" is from the global pointer
> "cmd->device->host->hostdata[0]", regardless of the possible free
> of "pHba", that original global pointer is never nullified and
> thus may become a dangling pointer and be dereferenced later,
> potentially causing a use-after-free.
>
> Fix the issue by nullifying "cmd->device->host->hostdata[0]" if
> adpt_hba_reset() returns a negative error code to __adpt_reset(),
> which indicates the free of "pHba". Also add a NULL check before
> any dereference of "pHba", or the aliased global pointer. Note
> that the similar NULL check already exists at other places, like
> in adpt_queue_lck().
>
> Signed-off-by: Hang Zhang <zh.nvgt@gmail.com>
> ---
>  drivers/scsi/dpt_i2o.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
> index 2e9155ba7408..9827517a1898 100644
> --- a/drivers/scsi/dpt_i2o.c
> +++ b/drivers/scsi/dpt_i2o.c
> @@ -753,6 +753,9 @@ static int __adpt_reset(struct scsi_cmnd* cmd)
>         char name[32];
>
>         pHba = (adpt_hba*)cmd->device->host->hostdata[0];
> +       if (!pHba) {
> +               return FAILED;
> +       }
>         strncpy(name, pHba->name, sizeof(name));
>         printk(KERN_WARNING"%s: Hba Reset: scsi id %d: tid: %d\n", name, cmd->device->channel, pHba->channel[cmd->device->channel].tid);
>         rcode =  adpt_hba_reset(pHba);
> @@ -760,6 +763,7 @@ static int __adpt_reset(struct scsi_cmnd* cmd)
>                 printk(KERN_WARNING"%s: HBA reset complete\n", name);
>                 return SUCCESS;
>         } else {
> +               cmd->device->host->hostdata[0] = NULL;
>                 printk(KERN_WARNING"%s: HBA reset failed (%x)\n", name, rcode);
>                 return FAILED;
>         }
> --
> 2.39.0
>

Hi James and Martin, could we get some feedback from you
regarding this patch? We want to clarify that this issue was
originally detected by our static analyzer, so we do not have
PoC or dynamic execution traces. However we do have a careful
manual code review to confirm the issue, and we think that
it seems apparent that the global pointer should be cleared after
its points-to object gets freed (and checked before use), which
is not the case in the current __adpt_reset() function. Please
feel free to let us know if we missed anything here, thank you
very much!

Best,
Hang