| Message ID | 20221227001001.51626-1-zh.nvgt@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: dpt_i2o: fix a potential use-after-free in __adpt_reset() | expand |
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
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; > } > >
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; > } > > >
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; }
__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(+)