| Message ID | 20220901113406.65876-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v1,1/1] platform/x86: p2sb: Fix UAF when caller uses resource name | expand |
Hi, On 9/1/22 13:34, Andy Shevchenko wrote: > We have to copy only selected fields from the original resource. > Because a PCI device will be removed immediately after getting > its resources, we may not use any allocated data, hence we may > not copy any pointers. > > Consider the following scenario: > > 1/ a caller of p2sb_bar() gets the resource; > > 2/ the resource has been copied by platform_device_add_data() > in order to create a platform device; > > 3/ the platform device creation will call for the device driver's > ->probe() as soon as a match found; > > 4/ the ->probe() takes given resources (see 2/) and tries to > access one of its field, i.e. 'name', in the > __devm_ioremap_resource() to create a pretty looking output; > > 5/ but the 'name' is a dangling pointer because p2sb_bar() > removed a PCI device, which 'name' had been copied to > the caller's memory. > > 6/ UAF (Use-After-Free) as a result. > > Kudos to Mika for the initial analisys of the issue. > > Fixes: 9745fb07474f ("platform/x86/intel: Add Primary to Sideband (P2SB) bridge support") > Reported-by: kernel test robot <oliver.sang@intel.com> > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Link: https://lore.kernel.org/linux-i2c/YvPCbnKqDiL2XEKp@xsang-OptiPlex-9020/ > Link: https://lore.kernel.org/linux-i2c/YtjAswDKfiuDfWYs@xsang-OptiPlex-9020/ > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/p2sb.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c > index fb2e141f3eb8..384d0962ae93 100644 > --- a/drivers/platform/x86/p2sb.c > +++ b/drivers/platform/x86/p2sb.c > @@ -42,10 +42,24 @@ static int p2sb_get_devfn(unsigned int *devfn) > return 0; > } > > +/* Copy resource from the first BAR of the device in question */ > static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) > { > - /* Copy resource from the first BAR of the device in question */ > - *mem = pdev->resource[0]; > + struct resource *bar0 = &pdev->resource[0]; > + > + /* Make sure we have no dangling pointers in the output */ > + memset(mem, 0, sizeof(*mem)); > + > + /* > + * We copy only selected fields from the original resource. > + * Because a PCI device will be removed soon, we may not use > + * any allocated data, hence we may not copy any pointers. > + */ > + mem->start = bar0->start; > + mem->end = bar0->end; > + mem->flags = bar0->flags; > + mem->desc = bar0->desc; > + > return 0; > } >
diff --git a/drivers/platform/x86/p2sb.c b/drivers/platform/x86/p2sb.c index fb2e141f3eb8..384d0962ae93 100644 --- a/drivers/platform/x86/p2sb.c +++ b/drivers/platform/x86/p2sb.c @@ -42,10 +42,24 @@ static int p2sb_get_devfn(unsigned int *devfn) return 0; } +/* Copy resource from the first BAR of the device in question */ static int p2sb_read_bar0(struct pci_dev *pdev, struct resource *mem) { - /* Copy resource from the first BAR of the device in question */ - *mem = pdev->resource[0]; + struct resource *bar0 = &pdev->resource[0]; + + /* Make sure we have no dangling pointers in the output */ + memset(mem, 0, sizeof(*mem)); + + /* + * We copy only selected fields from the original resource. + * Because a PCI device will be removed soon, we may not use + * any allocated data, hence we may not copy any pointers. + */ + mem->start = bar0->start; + mem->end = bar0->end; + mem->flags = bar0->flags; + mem->desc = bar0->desc; + return 0; }
We have to copy only selected fields from the original resource. Because a PCI device will be removed immediately after getting its resources, we may not use any allocated data, hence we may not copy any pointers. Consider the following scenario: 1/ a caller of p2sb_bar() gets the resource; 2/ the resource has been copied by platform_device_add_data() in order to create a platform device; 3/ the platform device creation will call for the device driver's ->probe() as soon as a match found; 4/ the ->probe() takes given resources (see 2/) and tries to access one of its field, i.e. 'name', in the __devm_ioremap_resource() to create a pretty looking output; 5/ but the 'name' is a dangling pointer because p2sb_bar() removed a PCI device, which 'name' had been copied to the caller's memory. 6/ UAF (Use-After-Free) as a result. Kudos to Mika for the initial analisys of the issue. Fixes: 9745fb07474f ("platform/x86/intel: Add Primary to Sideband (P2SB) bridge support") Reported-by: kernel test robot <oliver.sang@intel.com> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com> Link: https://lore.kernel.org/linux-i2c/YvPCbnKqDiL2XEKp@xsang-OptiPlex-9020/ Link: https://lore.kernel.org/linux-i2c/YtjAswDKfiuDfWYs@xsang-OptiPlex-9020/ Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/platform/x86/p2sb.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)