Re: [PATCH v2] PCI/DOE: Fix double free of a duplicate feature's sysfs name

[Jonathan Cameron <jic23@kernel.org>]

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