MIME-Version: 1.0
References: <20221227001001.51626-1-zh.nvgt@gmail.com> <9b0f6779-e2a5-54f1-2b9c-6bc42c6d3d90@redhat.com>
 <CAO2zrtbaz2XF+vHW6mC_+gUNwwkWffupbp2jg8_sWgcguy5AKA@mail.gmail.com> <859bdf0d-8679-e4aa-5c0e-49ead7e62059@redhat.com>
In-Reply-To: <859bdf0d-8679-e4aa-5c0e-49ead7e62059@redhat.com>
From:   Hang Zhang <zh.nvgt@gmail.com>
Date:   Tue, 7 Feb 2023 11:28:32 -0500
Message-ID: <CAO2zrtYgcYJRM-P0JHbCofjxmpxWCO=3Wqo7AiW1pZ-7nZ_=4g@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 Tue, Feb 7, 2023 at 10:34 AM Tomas Henzl <thenzl@redhat.com> wrote:
>
> On 2/3/23 21:05, Hang Zhang wrote:
>
> 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!
>
> It is now removed from the mainline, it can stay elsewhere
> for an uncertain time.
> If there is a mechanism how to fix patches only in those longterm kernels I don't know.
> Ask in related mailing lists.
> I think that when the driver is now eol and this issue hasn't been noticed
> before, it's likely not worth the effort to fix it now.
>
Hi Tomas, thank you for the reply. We will try to gather more information
about this. Thanks!

Best,
Hang
>
> 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;
>   }
>
>
>