| Message ID | 20261003102221.59894-1-donggeunyoo.kernel@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] PCI/DOE: Fix double free of a duplicate feature's sysfs name | expand |
> When a DOE feature is reported twice, sysfs_add_file_to_group() fails > with -EEXIST for the second copy and pci_doe_sysfs_feature_populate() > frees that attribute's name, but leaves the pointer in place. > pci_doe_sysfs_feature_remove() frees every name again when the device > is removed, or when a later feature fails to register, so the name is > freed twice: > > BUG: KASAN: double-free in pci_doe_sysfs_feature_remove+0x117/0x1b0 > Call Trace: > kfree+0x11a/0x420 > pci_doe_sysfs_feature_remove+0x117/0x1b0 > pci_doe_sysfs_teardown+0x90/0xf0 > pci_remove_bus_device+0x128/0x2e0 > pci_stop_and_remove_bus_device_locked+0x1d/0x30 > remove_store+0xd2/0xf0 > > Clear the pointer after freeing the name. > > Fixes: 2311ab1820fe ("PCI/DOE: Expose DOE features via sysfs") > Cc: stable@vger.kernel.org > Suggested-by: Alistair Francis <alistair@alistair23.me> > Assisted-by: LLM > Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com> Sashiko has reviewed this patch and found no issues. It looks great!
On Sat, 3 Oct 2026 19:22:21 +0900 Donggeun Yoo <donggeunyoo.kernel@gmail.com> wrote: > When a DOE feature is reported twice, sysfs_add_file_to_group() fails > with -EEXIST for the second copy and pci_doe_sysfs_feature_populate() > frees that attribute's name, but leaves the pointer in place. > pci_doe_sysfs_feature_remove() frees every name again when the device > is removed, or when a later feature fails to register, so the name is > freed twice: > > BUG: KASAN: double-free in pci_doe_sysfs_feature_remove+0x117/0x1b0 > Call Trace: > kfree+0x11a/0x420 > pci_doe_sysfs_feature_remove+0x117/0x1b0 > pci_doe_sysfs_teardown+0x90/0xf0 > pci_remove_bus_device+0x128/0x2e0 > pci_stop_and_remove_bus_device_locked+0x1d/0x30 > remove_store+0xd2/0xf0 > > Clear the pointer after freeing the name. > > Fixes: 2311ab1820fe ("PCI/DOE: Expose DOE features via sysfs") > Cc: stable@vger.kernel.org > Suggested-by: Alistair Francis <alistair@alistair23.me> > Assisted-by: LLM > Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com> > --- > Changes in v2: > - Keep freeing the duplicate's name in pci_doe_sysfs_feature_populate() > and clear the pointer, instead of leaving it to > pci_doe_sysfs_feature_remove() (Alistair Francis) > > v1: https://lore.kernel.org/all/20260928035035.252394-1-donggeunyoo.kernel@gmail.com/ > > Tested under QEMU (q35, cxl-type3, KASAN) on 72d3fcf802c4. The device > reports its CDAT feature once, so a test-only change either registers > its DOE capability as a second mailbox or inserts the feature N extra > times into one: > > case before after > no duplicate, remove no report no report > no duplicate, a name fails at probe no report no report > two mailboxes, remove double free no report This one and related are valid cases that real hardware might well do. > two mailboxes, 3 remove/rescan cycles 3 double frees no report > 3 duplicates in one mailbox, remove 3 double frees no report > 2 duplicates, a later name fails, 2 double frees no report > probed twice So this one is a hardware bug I think. I guess maybe the DOE spec may not strictly say you can't do this (I'm on wrong computer so haven't checked) but it would be very odd. Overall I'm never keen on code that relies on an error path to smooth over a valid condition. We can't deduplicate in the xarrays for the multimailbox case unless we do a combine of the xarrays and use that instead for the registration. That route would be a rather heavy weight fix though so I guess I don't mind this one that much. So argued myself around to this approach. Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com> > > The doe_features listing is the same before and after in every case. > > drivers/pci/doe.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index ac95b1d2d9997..6b5d9ba8e9697 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -208,8 +208,9 @@ static int pci_doe_sysfs_feature_populate(struct pci_dev *pdev, > pci_warn(pdev, "Failed adding %s to sysfs group\n", > attrs[i].attr.name); > goto fail; > - } else > - kfree(attrs[i].attr.name); > + } > + kfree(attrs[i].attr.name); > + attrs[i].attr.name = NULL; > } > } >
On Sat, 2026-10-03 at 19:22 +0900, Donggeun Yoo wrote: > When a DOE feature is reported twice, sysfs_add_file_to_group() fails > with -EEXIST for the second copy and pci_doe_sysfs_feature_populate() > frees that attribute's name, but leaves the pointer in place. > pci_doe_sysfs_feature_remove() frees every name again when the device > is removed, or when a later feature fails to register, so the name is > freed twice: > > BUG: KASAN: double-free in pci_doe_sysfs_feature_remove+0x117/0x1b0 > Call Trace: > kfree+0x11a/0x420 > pci_doe_sysfs_feature_remove+0x117/0x1b0 > pci_doe_sysfs_teardown+0x90/0xf0 > pci_remove_bus_device+0x128/0x2e0 > pci_stop_and_remove_bus_device_locked+0x1d/0x30 > remove_store+0xd2/0xf0 > > Clear the pointer after freeing the name. > > Fixes: 2311ab1820fe ("PCI/DOE: Expose DOE features via sysfs") > Cc: stable@vger.kernel.org > Suggested-by: Alistair Francis <alistair@alistair23.me> > Assisted-by: LLM > Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com> Reviewed-by: Alistair Francis <alistair@alistair23.me> Alistair > --- > Changes in v2: > - Keep freeing the duplicate's name in > pci_doe_sysfs_feature_populate() > and clear the pointer, instead of leaving it to > pci_doe_sysfs_feature_remove() (Alistair Francis) > > v1: > https://lore.kernel.org/all/20260928035035.252394-1-donggeunyoo.kernel@gmail.com/ > > Tested under QEMU (q35, cxl-type3, KASAN) on 72d3fcf802c4. The device > reports its CDAT feature once, so a test-only change either registers > its DOE capability as a second mailbox or inserts the feature N extra > times into one: > > case before after > no duplicate, remove no report no report > no duplicate, a name fails at probe no report no report > two mailboxes, remove double free no report > two mailboxes, 3 remove/rescan cycles 3 double frees no report > 3 duplicates in one mailbox, remove 3 double frees no report > 2 duplicates, a later name fails, 2 double frees no report > probed twice > > The doe_features listing is the same before and after in every case. > > drivers/pci/doe.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index ac95b1d2d9997..6b5d9ba8e9697 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -208,8 +208,9 @@ static int pci_doe_sysfs_feature_populate(struct > pci_dev *pdev, > pci_warn(pdev, "Failed adding %s to > sysfs group\n", > attrs[i].attr.name); > goto fail; > - } else > - kfree(attrs[i].attr.name); > + } > + kfree(attrs[i].attr.name); > + attrs[i].attr.name = NULL; > } > } >
On Sat, Oct 03, 2026 at 07:22:21PM +0900, Donggeun Yoo wrote: > When a DOE feature is reported twice, sysfs_add_file_to_group() fails > with -EEXIST for the second copy and pci_doe_sysfs_feature_populate() > frees that attribute's name, but leaves the pointer in place. > pci_doe_sysfs_feature_remove() frees every name again when the device > is removed, or when a later feature fails to register, so the name is > freed twice: > > BUG: KASAN: double-free in pci_doe_sysfs_feature_remove+0x117/0x1b0 > Call Trace: > kfree+0x11a/0x420 > pci_doe_sysfs_feature_remove+0x117/0x1b0 > pci_doe_sysfs_teardown+0x90/0xf0 > pci_remove_bus_device+0x128/0x2e0 > pci_stop_and_remove_bus_device_locked+0x1d/0x30 > remove_store+0xd2/0xf0 > > Clear the pointer after freeing the name. > > Fixes: 2311ab1820fe ("PCI/DOE: Expose DOE features via sysfs") > Cc: stable@vger.kernel.org > Suggested-by: Alistair Francis <alistair@alistair23.me> > Assisted-by: LLM > Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com> Applied to pci/doe for v7.4, thanks! > --- > Changes in v2: > - Keep freeing the duplicate's name in pci_doe_sysfs_feature_populate() > and clear the pointer, instead of leaving it to > pci_doe_sysfs_feature_remove() (Alistair Francis) > > v1: https://lore.kernel.org/all/20260928035035.252394-1-donggeunyoo.kernel@gmail.com/ > > Tested under QEMU (q35, cxl-type3, KASAN) on 72d3fcf802c4. The device > reports its CDAT feature once, so a test-only change either registers > its DOE capability as a second mailbox or inserts the feature N extra > times into one: > > case before after > no duplicate, remove no report no report > no duplicate, a name fails at probe no report no report > two mailboxes, remove double free no report > two mailboxes, 3 remove/rescan cycles 3 double frees no report > 3 duplicates in one mailbox, remove 3 double frees no report > 2 duplicates, a later name fails, 2 double frees no report > probed twice > > The doe_features listing is the same before and after in every case. > > drivers/pci/doe.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index ac95b1d2d9997..6b5d9ba8e9697 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -208,8 +208,9 @@ static int pci_doe_sysfs_feature_populate(struct pci_dev *pdev, > pci_warn(pdev, "Failed adding %s to sysfs group\n", > attrs[i].attr.name); > goto fail; > - } else > - kfree(attrs[i].attr.name); > + } > + kfree(attrs[i].attr.name); > + attrs[i].attr.name = NULL; > } > } > > -- > 2.53.0 >
diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c index ac95b1d2d9997..6b5d9ba8e9697 100644 --- a/drivers/pci/doe.c +++ b/drivers/pci/doe.c @@ -208,8 +208,9 @@ static int pci_doe_sysfs_feature_populate(struct pci_dev *pdev, pci_warn(pdev, "Failed adding %s to sysfs group\n", attrs[i].attr.name); goto fail; - } else - kfree(attrs[i].attr.name); + } + kfree(attrs[i].attr.name); + attrs[i].attr.name = NULL; } }
When a DOE feature is reported twice, sysfs_add_file_to_group() fails with -EEXIST for the second copy and pci_doe_sysfs_feature_populate() frees that attribute's name, but leaves the pointer in place. pci_doe_sysfs_feature_remove() frees every name again when the device is removed, or when a later feature fails to register, so the name is freed twice: BUG: KASAN: double-free in pci_doe_sysfs_feature_remove+0x117/0x1b0 Call Trace: kfree+0x11a/0x420 pci_doe_sysfs_feature_remove+0x117/0x1b0 pci_doe_sysfs_teardown+0x90/0xf0 pci_remove_bus_device+0x128/0x2e0 pci_stop_and_remove_bus_device_locked+0x1d/0x30 remove_store+0xd2/0xf0 Clear the pointer after freeing the name. Fixes: 2311ab1820fe ("PCI/DOE: Expose DOE features via sysfs") Cc: stable@vger.kernel.org Suggested-by: Alistair Francis <alistair@alistair23.me> Assisted-by: LLM Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com> --- Changes in v2: - Keep freeing the duplicate's name in pci_doe_sysfs_feature_populate() and clear the pointer, instead of leaving it to pci_doe_sysfs_feature_remove() (Alistair Francis) v1: https://lore.kernel.org/all/20260928035035.252394-1-donggeunyoo.kernel@gmail.com/ Tested under QEMU (q35, cxl-type3, KASAN) on 72d3fcf802c4. The device reports its CDAT feature once, so a test-only change either registers its DOE capability as a second mailbox or inserts the feature N extra times into one: case before after no duplicate, remove no report no report no duplicate, a name fails at probe no report no report two mailboxes, remove double free no report two mailboxes, 3 remove/rescan cycles 3 double frees no report 3 duplicates in one mailbox, remove 3 double frees no report 2 duplicates, a later name fails, 2 double frees no report probed twice The doe_features listing is the same before and after in every case. drivers/pci/doe.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)