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

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

[Hang Zhang]

__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

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

[Hang Zhang]

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

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

[Hang Zhang]

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;
>   }
>
>

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@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;
>   }
>
>
>