Ever since the generic platform device code started allocating DMA masks
itself the code to allocate and leak a private DMA mask in
platform_device_register_full has been superflous. More so the fact that
it unconditionally frees the DMA mask allocation in the failure path
can lead to slab corruption if the function fails later on for a device
where it didn't allocate the mask. Just remove the offending code.
Fixes: cdfee5623290 ("driver core: initialize a default DMA mask for platform device")
Reported-by: Artem S. Tashkinov <[email protected]>
Tested-by: Artem S. Tashkinov <[email protected]>
---
drivers/base/platform.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 7fa654f1288b..47d3e6187a1c 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -662,19 +662,6 @@ struct platform_device *platform_device_register_full(
pdev->dev.of_node_reused = pdevinfo->of_node_reused;
if (pdevinfo->dma_mask) {
- /*
- * This memory isn't freed when the device is put,
- * I don't have a nice idea for that though. Conceptually
- * dma_mask in struct device should not be a pointer.
- * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
- */
- pdev->dev.dma_mask =
- kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
- if (!pdev->dev.dma_mask)
- goto err;
-
- kmemleak_ignore(pdev->dev.dma_mask);
-
*pdev->dev.dma_mask = pdevinfo->dma_mask;
pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
}
@@ -700,7 +687,6 @@ struct platform_device *platform_device_register_full(
if (ret) {
err:
ACPI_COMPANION_SET(&pdev->dev, NULL);
- kfree(pdev->dev.dma_mask);
platform_device_put(pdev);
return ERR_PTR(ret);
}
--
2.24.1
On 3/11/20 4:07 PM, Christoph Hellwig wrote:
> Ever since the generic platform device code started allocating DMA masks
> itself the code to allocate and leak a private DMA mask in
> platform_device_register_full has been superflous. More so the fact that
> it unconditionally frees the DMA mask allocation in the failure path
> can lead to slab corruption if the function fails later on for a device
> where it didn't allocate the mask. Just remove the offending code.
>
> Fixes: cdfee5623290 ("driver core: initialize a default DMA mask for platform device")
> Reported-by: Artem S. Tashkinov <[email protected]>
> Tested-by: Artem S. Tashkinov <[email protected]>
> ---
> drivers/base/platform.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 7fa654f1288b..47d3e6187a1c 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -662,19 +662,6 @@ struct platform_device *platform_device_register_full(
> pdev->dev.of_node_reused = pdevinfo->of_node_reused;
>
> if (pdevinfo->dma_mask) {
> - /*
> - * This memory isn't freed when the device is put,
> - * I don't have a nice idea for that though. Conceptually
> - * dma_mask in struct device should not be a pointer.
> - * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
> - */
> - pdev->dev.dma_mask =
> - kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> - if (!pdev->dev.dma_mask)
> - goto err;
> -
> - kmemleak_ignore(pdev->dev.dma_mask);
> -
> *pdev->dev.dma_mask = pdevinfo->dma_mask;
> pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
> }
> @@ -700,7 +687,6 @@ struct platform_device *platform_device_register_full(
> if (ret) {
> err:
> ACPI_COMPANION_SET(&pdev->dev, NULL);
> - kfree(pdev->dev.dma_mask);
> platform_device_put(pdev);
> return ERR_PTR(ret);
> }
>
I'd love to see this patch applied to the next round of stable kernels
(at the very least 5.5.x) ASAP because I've been stuck with kernel 5.3
on Fedora for quite some time already.
Thank you!
On Wed, Mar 11, 2020 at 05:07:10PM +0100, Christoph Hellwig wrote:
> Ever since the generic platform device code started allocating DMA masks
> itself the code to allocate and leak a private DMA mask in
> platform_device_register_full has been superflous. More so the fact that
> it unconditionally frees the DMA mask allocation in the failure path
> can lead to slab corruption if the function fails later on for a device
> where it didn't allocate the mask. Just remove the offending code.
>
> Fixes: cdfee5623290 ("driver core: initialize a default DMA mask for platform device")
> Reported-by: Artem S. Tashkinov <[email protected]>
> Tested-by: Artem S. Tashkinov <[email protected]>
No s-o-b from you? :(
I can take this, or Linus, you can take this now if you want to as well:
Reviewed-by: Greg Kroah-Hartman <[email protected]>
On Wed, Mar 11, 2020 at 05:14:23PM +0100, Greg KH wrote:
> On Wed, Mar 11, 2020 at 05:07:10PM +0100, Christoph Hellwig wrote:
> > Ever since the generic platform device code started allocating DMA masks
> > itself the code to allocate and leak a private DMA mask in
> > platform_device_register_full has been superflous. More so the fact that
> > it unconditionally frees the DMA mask allocation in the failure path
> > can lead to slab corruption if the function fails later on for a device
> > where it didn't allocate the mask. Just remove the offending code.
> >
> > Fixes: cdfee5623290 ("driver core: initialize a default DMA mask for platform device")
> > Reported-by: Artem S. Tashkinov <[email protected]>
> > Tested-by: Artem S. Tashkinov <[email protected]>
>
> No s-o-b from you? :(
>
> I can take this, or Linus, you can take this now if you want to as well:
Sorry, here it is:
Signed-off-by: Christoph Hellwig <[email protected]>
On 11/03/2020 4:07 pm, Christoph Hellwig wrote:
> Ever since the generic platform device code started allocating DMA masks
> itself the code to allocate and leak a private DMA mask in
> platform_device_register_full has been superflous. More so the fact that
> it unconditionally frees the DMA mask allocation in the failure path
> can lead to slab corruption if the function fails later on for a device
> where it didn't allocate the mask. Just remove the offending code.
I'm sure I mentioned this in passing at the time, but only in the
context of a cleanup; I never noticed it could be cause for an actual bug :)
Reviewed-by: Robin Murphy <[email protected]>
> Fixes: cdfee5623290 ("driver core: initialize a default DMA mask for platform device")
> Reported-by: Artem S. Tashkinov <[email protected]>
> Tested-by: Artem S. Tashkinov <[email protected]>
> ---
> drivers/base/platform.c | 14 --------------
> 1 file changed, 14 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 7fa654f1288b..47d3e6187a1c 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -662,19 +662,6 @@ struct platform_device *platform_device_register_full(
> pdev->dev.of_node_reused = pdevinfo->of_node_reused;
>
> if (pdevinfo->dma_mask) {
> - /*
> - * This memory isn't freed when the device is put,
> - * I don't have a nice idea for that though. Conceptually
> - * dma_mask in struct device should not be a pointer.
> - * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
> - */
> - pdev->dev.dma_mask =
> - kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> - if (!pdev->dev.dma_mask)
> - goto err;
> -
> - kmemleak_ignore(pdev->dev.dma_mask);
> -
> *pdev->dev.dma_mask = pdevinfo->dma_mask;
> pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
> }
> @@ -700,7 +687,6 @@ struct platform_device *platform_device_register_full(
> if (ret) {
> err:
> ACPI_COMPANION_SET(&pdev->dev, NULL);
> - kfree(pdev->dev.dma_mask);
> platform_device_put(pdev);
> return ERR_PTR(ret);
> }
>
On Wed, Mar 11, 2020 at 05:15:51PM +0100, Christoph Hellwig wrote:
> On Wed, Mar 11, 2020 at 05:14:23PM +0100, Greg KH wrote:
> > On Wed, Mar 11, 2020 at 05:07:10PM +0100, Christoph Hellwig wrote:
> > > Ever since the generic platform device code started allocating DMA masks
> > > itself the code to allocate and leak a private DMA mask in
> > > platform_device_register_full has been superflous. More so the fact that
> > > it unconditionally frees the DMA mask allocation in the failure path
> > > can lead to slab corruption if the function fails later on for a device
> > > where it didn't allocate the mask. Just remove the offending code.
> > >
> > > Fixes: cdfee5623290 ("driver core: initialize a default DMA mask for platform device")
> > > Reported-by: Artem S. Tashkinov <[email protected]>
> > > Tested-by: Artem S. Tashkinov <[email protected]>
> >
> > No s-o-b from you? :(
> >
> > I can take this, or Linus, you can take this now if you want to as well:
>
> Sorry, here it is:
>
> Signed-off-by: Christoph Hellwig <[email protected]>
Is this still needed with the patch that Linus just committed to his
tree?
thanks,
greg k-h
On Wed, Mar 11, 2020 at 06:18:02PM +0100, Greg KH wrote:
> > Sorry, here it is:
> >
> > Signed-off-by: Christoph Hellwig <[email protected]>
>
> Is this still needed with the patch that Linus just committed to his
> tree?
No.
On Wed, Mar 11, 2020 at 10:18 AM Greg KH <[email protected]> wrote:
>
> Is this still needed with the patch that Linus just committed to his
> tree?
My patch is basically the same, just with the field renamed too, and
not blindly just assigning to "*pdev->dev.dma_mask" (my variant does
pdev->platform_dma_mask = pdevinfo->dma_mask;
pdev->dev.dma_mask = &pdev->platform_dma_mask;
instead of that incomprehensible
*pdev->dev.dma_mask = pdevinfo->dma_mask;
which depends on that dev.dma_mask pointer having been initialized in
a random place earlier).
I had the cleanups (uncommited) in my tree, and just removed the
kfree() as per Christoph.
Linus