Re: [PATCH] scsi: dpt_i2o: fix a potential use-after-free in __adpt_reset()

[Hang Zhang]

On Tue, Feb 7, 2023 at 10:34 AM Tomas Henzl <thenzl@xxxxxxxxxx> wrote:
>
> On 2/3/23 21:05, Hang Zhang wrote:
>
> On Fri, Feb 3, 2023 at 11:07 AM Tomas Henzl <thenzl@xxxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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;
>   }
>
>
>