Hi Bjorn,
Yes, I get segmentation fault on unloading custom module during the check of error handling case.
There is no directly visible access to res_attr fields, as you mentioned, other than the one in a call chain after a check in pci_remove_resource_files() which seems to cause the issue (accessing name).
Load and unload module will invoke pci_enable_sriov() and disable_pci_sriov() respectively that are both having a call of pci_remove_resource_files() in call chain.
In that function existing check is not behaving as expected since memory is freed but pointers left dangling.
Below is call trace and detail description.
During loading of module pci_enable_sriov() is called, I have following invoking sequence:
device_create_file
pci_create_capabilities_sysfs
pci_create_sysfs_dev_files
pci_bus_add_device
pci_iov_add_virtfn
sriov_enable
pci_enable_sriov
In case of a failure of device_create_file() for dev_attr_reset case (which was the case I tested), it will call pci_remove_resource_files(), which will free both, res_attr and res_attr_wc array elements (pointers) without setting them to NULL.
Then failure is propagated only up to pci_create_sysfs_dev_files and return value is not checked inside of pci_bus_add_device.
That causes the same behavior as pci_enable_sriov function passed.
On unload, during pci_disable_sriov(), pci_remove_resource_files() will be regularly called to try to remove the files.
The check segment that currently exist will not prevent calling of removal code since pointers are left as dangling pointers, even though the resource files and attributes are already freed.
res_attr = pdev->res_attr[i];
if (res_attr) {
sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
kfree(res_attr);
}
Attribute res_attr[i]->name is the one causing segmentation fault when strlen() function is called in kernfs_name_hash().
Here is the call trace:
[ 991.796300] RIP: 0010:strlen+0x0/0x30
[ 991.807240] Code: f8 48 89 fa 48 89 e5 74 09 48 83 c2 01 80 3a 00 75 f7 48 83 c6 01 0f b6 4e ff 48 83 c2 01 84 c9 88 4a ff 75 ed 5d c3 0f 1f 00 <80> 3f 00 55 48 89 e5 74 14 48 89 f8 48 83 c7 01 80 3f 00 75 f7 48
[ 991.863423] RSP: 0018:ffffc1d68a9ffb80 EFLAGS: 00010246
[ 991.879051] RAX: 0000000000000000 RBX: b5f8d4ce837e84cb RCX: 0000000000000000
[ 991.900395] RDX: 0000000000000000 RSI: 0000000000000000 RDI: b5f8d4ce837e84cb
[ 991.921739] RBP: ffffc1d68a9ffb98 R08: 0000000000000000 R09: ffffffff96964e01
[ 991.943086] R10: ffffc1d68a9ffb78 R11: 0000000000000001 R12: 0000000000000000
[ 991.964432] R13: 0000000000000000 R14: b5f8d4ce837e84cb R15: 0000000000000038
[ 991.985777] FS: 00007f3c278b3740(0000) GS:ffffa0a61f740000(0000) knlGS:0000000000000000
[ 992.009983] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 992.027168] CR2: 00007f3c27920340 CR3: 000000074c010004 CR4: 00000000003606e0
[ 992.048516] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 992.069861] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 992.091205] Call Trace:
[ 992.098523] ? kernfs_name_hash+0x17/0x80
[ 992.110499] kernfs_find_ns+0x3a/0xc0
[ 992.121448] kernfs_remove_by_name_ns+0x36/0xa0
[ 992.134994] sysfs_remove_bin_file+0x17/0x20
[ 992.147760] pci_remove_resource_files+0x38/0x80
[ 992.161565] pci_remove_sysfs_dev_files+0x5b/0xc0
[ 992.175631] pci_stop_bus_device+0x78/0x90
[ 992.187875] pci_stop_and_remove_bus_device+0x12/0x20
[ 992.202983] pci_iov_remove_virtfn+0xc3/0x110
[ 992.216006] sriov_disable+0x43/0x100
[ 992.226953] pci_disable_sriov+0x23/0x30
BR,
Danijel Slivka
-----Original Message-----
From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
Sent: Saturday, May 8, 2021 12:07 AM
To: Slivka, Danijel <Danijel.Slivka@xxxxxxx>
Cc: bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] PCI: Fix accessing freed memory in pci_remove_resource_files
Hi Danijel,
Thanks for the patch.
On Fri, May 07, 2021 at 06:27:06PM +0800, Danijel Slivka wrote:
> This patch fixes segmentation fault during accessing already freed pci
> device resource files, as after freeing res_attr and res_attr_wc
> elements, in pci_remove_resource_files function, they are left as
> dangling pointers.
>
> Signed-off-by: Danijel Slivka <danijel.slivka@xxxxxxx>
> ---
> drivers/pci/pci-sysfs.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index
> f8afd54ca3e1..bbdf6c57fcda 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -1130,12 +1130,14 @@ static void pci_remove_resource_files(struct pci_dev *pdev)
> if (res_attr) {
> sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
> kfree(res_attr);
> + pdev->res_attr[i] = NULL;
> }
>
> res_attr = pdev->res_attr_wc[i];
> if (res_attr) {
> sysfs_remove_bin_file(&pdev->dev.kobj, res_attr);
> kfree(res_attr);
> + pdev->res_attr_wc[i] = NULL;
If this patch fixes something, I would expect to see a test like this
somewhere:
if (pdev->res_attr[i])
pdev->res_attr[i]->size = 0;
But I don't see anything like that, so I can't figure out where we actually use res_attr[i] or res_attr_wc[i], except in
pci_remove_resource_files() itself.
Did you actually see a segmentation fault? If so, where?
> }
> }
> }
> --
> 2.20.1
>
