From: Geert Uytterhoeven <geert@linux-m68k.org> To: Jacopo Mondi <jacopo+renesas@jmondi.org> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>, Rich Felker <dalias@libc.org>, Thomas Petazzoni <thomas.petazzoni@free-electrons.com>, Robin Murphy <robin.murphy@arm.com>, Linux-Renesas <linux-renesas-soc@vger.kernel.org>, Linux-sh list <linux-sh@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] sh: mm: Fix unprotected access to struct device Date: Tue, 17 Apr 2018 14:04:27 +0000 [thread overview] Message-ID: <CAMuHMdX876iXo-Mio-VxUmR9hq9WFhNtOAn_XdhjFgNvDvXJvg@mail.gmail.com> (raw) In-Reply-To: <1523972123-5700-1-git-send-email-jacopo+renesas@jmondi.org> Hi Jacopo, Thanks for your patch! On Tue, Apr 17, 2018 at 3:35 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping > operations observe dev->dma_pfn_offset") the generic DMA allocation > function on which the SH 'dma_alloc_coherent()' function relies on, > access the 'dma_pfn_offset' field of struct device. accesses > Unfortunately the 'dma_generic_alloc_coherent()' function is called from > several places with a NULL struct device argument, halting the CPU > during the boot process. > > This patch fixes the issue protecting access to dev->dma_pfn_offset, by protecting access to the > with a trivial check for validity. It also passes a valid 'struct device' > in the 'platform_resource_setup_memory' function which is the main user > of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future > (and existing) bogus users of this function they're should provide a valid drop "they're should"? > 'struct device' whenever possible. > --- a/arch/sh/mm/consistent.c > +++ b/arch/sh/mm/consistent.c > @@ -39,6 +39,8 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size, > void *ret, *ret_nocache; > int order = get_order(size); > > + WARN_ON(!dev); > + > gfp |= __GFP_ZERO; > > ret = (void *)__get_free_pages(gfp, order); > @@ -59,7 +61,9 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size, > > split_page(pfn_to_page(virt_to_phys(ret) >> PAGE_SHIFT), order); > > - *dma_handle = virt_to_phys(ret) - PFN_PHYS(dev->dma_pfn_offset); > + *dma_handle = virt_to_phys(ret); > + if (dev) > + *dma_handle -= PFN_PHYS(dev->dma_pfn_offset); I would keep the WARN_ON() and the (ideally unneeded) dev check as close to each other as possible: if (!WARN_ON(!dev)) *dma_handle -= PFN_PHYS(dev->dma_pfn_offset); > > return ret_nocache; > } > @@ -69,9 +73,14 @@ void dma_generic_free_coherent(struct device *dev, size_t size, > unsigned long attrs) > { > int order = get_order(size); > - unsigned long pfn = (dma_handle >> PAGE_SHIFT) + dev->dma_pfn_offset; > + unsigned long pfn = (dma_handle >> PAGE_SHIFT); > int k; > > + WARN_ON(!dev); > + > + if (dev) > + pfn += dev->dma_pfn_offset; if (!WARN_ON(!dev)) pfn += dev->dma_pfn_offset; > + > for (k = 0; k < (1 << order); k++) > __free_pages(pfn_to_page(pfn + k), 0); Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
WARNING: multiple messages have this Message-ID (diff)
From: Geert Uytterhoeven <geert@linux-m68k.org> To: Jacopo Mondi <jacopo+renesas@jmondi.org> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>, Rich Felker <dalias@libc.org>, Thomas Petazzoni <thomas.petazzoni@free-electrons.com>, Robin Murphy <robin.murphy@arm.com>, Linux-Renesas <linux-renesas-soc@vger.kernel.org>, Linux-sh list <linux-sh@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] sh: mm: Fix unprotected access to struct device Date: Tue, 17 Apr 2018 16:04:27 +0200 [thread overview] Message-ID: <CAMuHMdX876iXo-Mio-VxUmR9hq9WFhNtOAn_XdhjFgNvDvXJvg@mail.gmail.com> (raw) In-Reply-To: <1523972123-5700-1-git-send-email-jacopo+renesas@jmondi.org> Hi Jacopo, Thanks for your patch! On Tue, Apr 17, 2018 at 3:35 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote: > With commit ce88313069c36eef80f21fd7 ("arch/sh: make the DMA mapping > operations observe dev->dma_pfn_offset") the generic DMA allocation > function on which the SH 'dma_alloc_coherent()' function relies on, > access the 'dma_pfn_offset' field of struct device. accesses > Unfortunately the 'dma_generic_alloc_coherent()' function is called from > several places with a NULL struct device argument, halting the CPU > during the boot process. > > This patch fixes the issue protecting access to dev->dma_pfn_offset, by protecting access to the > with a trivial check for validity. It also passes a valid 'struct device' > in the 'platform_resource_setup_memory' function which is the main user > of 'dma_alloc_coherent()', and inserting a WARN_ON() check to make future > (and existing) bogus users of this function they're should provide a valid drop "they're should"? > 'struct device' whenever possible. > --- a/arch/sh/mm/consistent.c > +++ b/arch/sh/mm/consistent.c > @@ -39,6 +39,8 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size, > void *ret, *ret_nocache; > int order = get_order(size); > > + WARN_ON(!dev); > + > gfp |= __GFP_ZERO; > > ret = (void *)__get_free_pages(gfp, order); > @@ -59,7 +61,9 @@ void *dma_generic_alloc_coherent(struct device *dev, size_t size, > > split_page(pfn_to_page(virt_to_phys(ret) >> PAGE_SHIFT), order); > > - *dma_handle = virt_to_phys(ret) - PFN_PHYS(dev->dma_pfn_offset); > + *dma_handle = virt_to_phys(ret); > + if (dev) > + *dma_handle -= PFN_PHYS(dev->dma_pfn_offset); I would keep the WARN_ON() and the (ideally unneeded) dev check as close to each other as possible: if (!WARN_ON(!dev)) *dma_handle -= PFN_PHYS(dev->dma_pfn_offset); > > return ret_nocache; > } > @@ -69,9 +73,14 @@ void dma_generic_free_coherent(struct device *dev, size_t size, > unsigned long attrs) > { > int order = get_order(size); > - unsigned long pfn = (dma_handle >> PAGE_SHIFT) + dev->dma_pfn_offset; > + unsigned long pfn = (dma_handle >> PAGE_SHIFT); > int k; > > + WARN_ON(!dev); > + > + if (dev) > + pfn += dev->dma_pfn_offset; if (!WARN_ON(!dev)) pfn += dev->dma_pfn_offset; > + > for (k = 0; k < (1 << order); k++) > __free_pages(pfn_to_page(pfn + k), 0); Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
next prev parent reply other threads:[~2018-04-17 14:04 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-17 13:35 [PATCH] sh: mm: Fix unprotected access to struct device Jacopo Mondi 2018-04-17 13:35 ` Jacopo Mondi 2018-04-17 13:54 ` Thomas Petazzoni 2018-04-17 13:54 ` Thomas Petazzoni 2018-04-17 13:59 ` jacopo mondi 2018-04-17 13:59 ` jacopo mondi 2018-04-20 8:30 ` Christoph Hellwig 2018-04-20 8:30 ` Christoph Hellwig 2018-04-17 14:04 ` Geert Uytterhoeven [this message] 2018-04-17 14:04 ` Geert Uytterhoeven 2018-04-17 14:20 ` jacopo mondi 2018-04-17 14:20 ` jacopo mondi 2018-04-18 9:13 ` Sergei Shtylyov 2018-04-18 9:13 ` Sergei Shtylyov 2018-04-18 10:47 ` Christoph Hellwig 2018-04-18 10:47 ` Christoph Hellwig 2018-04-18 13:13 ` jacopo mondi 2018-04-18 13:13 ` jacopo mondi 2018-04-20 8:31 ` Christoph Hellwig 2018-04-20 8:31 ` Christoph Hellwig 2018-04-20 9:59 ` Geert Uytterhoeven 2018-04-20 9:59 ` Geert Uytterhoeven 2018-04-20 14:56 ` Rich Felker 2018-04-20 14:56 ` Rich Felker 2018-05-02 7:41 ` jacopo mondi 2018-05-02 7:41 ` jacopo mondi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAMuHMdX876iXo-Mio-VxUmR9hq9WFhNtOAn_XdhjFgNvDvXJvg@mail.gmail.com \ --to=geert@linux-m68k.org \ --cc=dalias@libc.org \ --cc=jacopo+renesas@jmondi.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-renesas-soc@vger.kernel.org \ --cc=linux-sh@vger.kernel.org \ --cc=robin.murphy@arm.com \ --cc=thomas.petazzoni@free-electrons.com \ --cc=ysato@users.sourceforge.jp \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.