MIME-Version: 1.0
References: <20221227001001.51626-1-zh.nvgt@gmail.com> <9b0f6779-e2a5-54f1-2b9c-6bc42c6d3d90@redhat.com>
In-Reply-To: <9b0f6779-e2a5-54f1-2b9c-6bc42c6d3d90@redhat.com>
From:   Hang Zhang <zh.nvgt@gmail.com>
Date:   Fri, 3 Feb 2023 15:05:47 -0500
Message-ID: <CAO2zrtbaz2XF+vHW6mC_+gUNwwkWffupbp2jg8_sWgcguy5AKA@mail.gmail.com>
Subject: Re: [PATCH] scsi: dpt_i2o: fix a potential use-after-free in __adpt_reset()
To:     Tomas Henzl <thenzl@redhat.com>
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"
Precedence: bulk

On Fri, Feb 3, 2023 at 11:07 AM Tomas Henzl <thenzl@redhat.com> wrote:
>
> Hi Hang,
>
> with b04e75a4a8a8 scsi: dpt_i2o: Remove obsolete driver
> was the driver removed from kernel.
>
> Regards,
> Tomas
>

Hi Tomas, thank you very much for pointing this out! I'm wondering
that is this removal for all the kernel versions, or there might still be
some maintained versions using this driver (e.g., from a quick check
it seems the longterm 5.15.91 still has the driver)? If so, do you think
this is a valid issue that we need to fix in some prior affected
longterm kernel versions? Thank you for your help and comments!

Best,
Hang

> On 12/27/22 01:10, Hang Zhang 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;
>   }
>
>