Hi Arnd,
can you please look at this patch?
Thanks,
Michal
On 06/03/2013 02:44 PM, Michal Simek wrote:
> Check that dma_ops are initialized correctly.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
> Functions dma_mmap_attrs(), dma_get_sgtable_attrs()
> already have this checking.
>
> ---
> include/asm-generic/dma-mapping-common.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/include/asm-generic/dma-mapping-common.h b/include/asm-generic/dma-mapping-common.h
> index de8bf89..d430cab 100644
> --- a/include/asm-generic/dma-mapping-common.h
> +++ b/include/asm-generic/dma-mapping-common.h
> @@ -16,6 +16,7 @@ static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
> dma_addr_t addr;
>
> kmemcheck_mark_initialized(ptr, size);
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> addr = ops->map_page(dev, virt_to_page(ptr),
> (unsigned long)ptr & ~PAGE_MASK, size,
> @@ -33,6 +34,7 @@ static inline void dma_unmap_single_attrs(struct device *dev, dma_addr_t addr,
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->unmap_page)
> ops->unmap_page(dev, addr, size, dir, attrs);
> @@ -49,6 +51,7 @@ static inline int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
>
> for_each_sg(sg, s, nents, i)
> kmemcheck_mark_initialized(sg_virt(s), s->length);
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> ents = ops->map_sg(dev, sg, nents, dir, attrs);
> debug_dma_map_sg(dev, sg, nents, ents, dir);
> @@ -62,6 +65,7 @@ static inline void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> debug_dma_unmap_sg(dev, sg, nents, dir);
> if (ops->unmap_sg)
> @@ -76,6 +80,7 @@ static inline dma_addr_t dma_map_page(struct device *dev, struct page *page,
> dma_addr_t addr;
>
> kmemcheck_mark_initialized(page_address(page) + offset, size);
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> addr = ops->map_page(dev, page, offset, size, dir, NULL);
> debug_dma_map_page(dev, page, offset, size, dir, addr, false);
> @@ -88,6 +93,7 @@ static inline void dma_unmap_page(struct device *dev, dma_addr_t addr,
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->unmap_page)
> ops->unmap_page(dev, addr, size, dir, NULL);
> @@ -100,6 +106,7 @@ static inline void dma_sync_single_for_cpu(struct device *dev, dma_addr_t addr,
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->sync_single_for_cpu)
> ops->sync_single_for_cpu(dev, addr, size, dir);
> @@ -112,6 +119,7 @@ static inline void dma_sync_single_for_device(struct device *dev,
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->sync_single_for_device)
> ops->sync_single_for_device(dev, addr, size, dir);
> @@ -126,6 +134,7 @@ static inline void dma_sync_single_range_for_cpu(struct device *dev,
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->sync_single_for_cpu)
> ops->sync_single_for_cpu(dev, addr + offset, size, dir);
> @@ -140,6 +149,7 @@ static inline void dma_sync_single_range_for_device(struct device *dev,
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->sync_single_for_device)
> ops->sync_single_for_device(dev, addr + offset, size, dir);
> @@ -152,6 +162,7 @@ dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->sync_sg_for_cpu)
> ops->sync_sg_for_cpu(dev, sg, nelems, dir);
> @@ -164,6 +175,7 @@ dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->sync_sg_for_device)
> ops->sync_sg_for_device(dev, sg, nelems, dir);
> --
> 1.8.2.3
>
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
[+cc Marek]
On Mon, Jun 3, 2013 at 6:44 AM, Michal Simek <[email protected]> wrote:
> Check that dma_ops are initialized correctly.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
> Functions dma_mmap_attrs(), dma_get_sgtable_attrs()
> already have this checking.
>
> ---
> include/asm-generic/dma-mapping-common.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/include/asm-generic/dma-mapping-common.h b/include/asm-generic/dma-mapping-common.h
> index de8bf89..d430cab 100644
> --- a/include/asm-generic/dma-mapping-common.h
> +++ b/include/asm-generic/dma-mapping-common.h
> @@ -16,6 +16,7 @@ static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
> dma_addr_t addr;
>
> kmemcheck_mark_initialized(ptr, size);
> + BUG_ON(!ops);
Does this actually help anything? I expected that if "ops" is NULL,
we would just oops anyway when we attempted to call "ops->map_page()"
because we already trap null pointer dereferences. At least, when I
tried leaving a pci_bus.ops pointer NULL, I got a nice panic and
backtrace even without adding an explicit BUG_ON().
I cc'd Marek, who added the similar BUG_ON()s in dma_mmap_attrs() and
dma_get_sgtable_attrs() with d2b7428eb0 and 64ccc9c033.
> BUG_ON(!valid_dma_direction(dir));
> addr = ops->map_page(dev, virt_to_page(ptr),
> (unsigned long)ptr & ~PAGE_MASK, size,
> @@ -33,6 +34,7 @@ static inline void dma_unmap_single_attrs(struct device *dev, dma_addr_t addr,
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->unmap_page)
> ops->unmap_page(dev, addr, size, dir, attrs);
> @@ -49,6 +51,7 @@ static inline int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg,
>
> for_each_sg(sg, s, nents, i)
> kmemcheck_mark_initialized(sg_virt(s), s->length);
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> ents = ops->map_sg(dev, sg, nents, dir, attrs);
> debug_dma_map_sg(dev, sg, nents, ents, dir);
> @@ -62,6 +65,7 @@ static inline void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> debug_dma_unmap_sg(dev, sg, nents, dir);
> if (ops->unmap_sg)
> @@ -76,6 +80,7 @@ static inline dma_addr_t dma_map_page(struct device *dev, struct page *page,
> dma_addr_t addr;
>
> kmemcheck_mark_initialized(page_address(page) + offset, size);
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> addr = ops->map_page(dev, page, offset, size, dir, NULL);
> debug_dma_map_page(dev, page, offset, size, dir, addr, false);
> @@ -88,6 +93,7 @@ static inline void dma_unmap_page(struct device *dev, dma_addr_t addr,
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->unmap_page)
> ops->unmap_page(dev, addr, size, dir, NULL);
> @@ -100,6 +106,7 @@ static inline void dma_sync_single_for_cpu(struct device *dev, dma_addr_t addr,
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->sync_single_for_cpu)
> ops->sync_single_for_cpu(dev, addr, size, dir);
> @@ -112,6 +119,7 @@ static inline void dma_sync_single_for_device(struct device *dev,
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->sync_single_for_device)
> ops->sync_single_for_device(dev, addr, size, dir);
> @@ -126,6 +134,7 @@ static inline void dma_sync_single_range_for_cpu(struct device *dev,
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->sync_single_for_cpu)
> ops->sync_single_for_cpu(dev, addr + offset, size, dir);
> @@ -140,6 +149,7 @@ static inline void dma_sync_single_range_for_device(struct device *dev,
> {
> const struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->sync_single_for_device)
> ops->sync_single_for_device(dev, addr + offset, size, dir);
> @@ -152,6 +162,7 @@ dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg,
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->sync_sg_for_cpu)
> ops->sync_sg_for_cpu(dev, sg, nelems, dir);
> @@ -164,6 +175,7 @@ dma_sync_sg_for_device(struct device *dev, struct scatterlist *sg,
> {
> struct dma_map_ops *ops = get_dma_ops(dev);
>
> + BUG_ON(!ops);
> BUG_ON(!valid_dma_direction(dir));
> if (ops->sync_sg_for_device)
> ops->sync_sg_for_device(dev, sg, nelems, dir);
> --
> 1.8.2.3
>
Hello,
On 6/11/2013 4:34 AM, Bjorn Helgaas wrote:
> [+cc Marek]
>
> On Mon, Jun 3, 2013 at 6:44 AM, Michal Simek <[email protected]> wrote:
> > Check that dma_ops are initialized correctly.
> >
> > Signed-off-by: Michal Simek <[email protected]>
> > ---
> > Functions dma_mmap_attrs(), dma_get_sgtable_attrs()
> > already have this checking.
> >
> > ---
> > include/asm-generic/dma-mapping-common.h | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/include/asm-generic/dma-mapping-common.h b/include/asm-generic/dma-mapping-common.h
> > index de8bf89..d430cab 100644
> > --- a/include/asm-generic/dma-mapping-common.h
> > +++ b/include/asm-generic/dma-mapping-common.h
> > @@ -16,6 +16,7 @@ static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
> > dma_addr_t addr;
> >
> > kmemcheck_mark_initialized(ptr, size);
> > + BUG_ON(!ops);
>
> Does this actually help anything? I expected that if "ops" is NULL,
> we would just oops anyway when we attempted to call "ops->map_page()"
> because we already trap null pointer dereferences. At least, when I
> tried leaving a pci_bus.ops pointer NULL, I got a nice panic and
> backtrace even without adding an explicit BUG_ON().
>
> I cc'd Marek, who added the similar BUG_ON()s in dma_mmap_attrs() and
> dma_get_sgtable_attrs() with d2b7428eb0 and 64ccc9c033.
I think that I've copied it from dma_alloc_coherent() in microblaze. You
are right that lack
of this check will also trigger oops in ops==NULL case, but I think that
adding explicit check
in all functions, which use it, is a good idea. It serves as a kind of
documentation and
emphasizes that missing ops is really an issue.
Best regards
--
Marek Szyprowski
Samsung R&D Institute Poland
On Tue, 2013-06-11 at 13:02 +0200, Marek Szyprowski wrote:
> Hello,
>
> On 6/11/2013 4:34 AM, Bjorn Helgaas wrote:
> > [+cc Marek]
> >
> > On Mon, Jun 3, 2013 at 6:44 AM, Michal Simek <[email protected]> wrote:
> > > Check that dma_ops are initialized correctly.
> > >
> > > Signed-off-by: Michal Simek <[email protected]>
> > > ---
> > > Functions dma_mmap_attrs(), dma_get_sgtable_attrs()
> > > already have this checking.
> > >
> > > ---
> > > include/asm-generic/dma-mapping-common.h | 12 ++++++++++++
> > > 1 file changed, 12 insertions(+)
> > >
> > > diff --git a/include/asm-generic/dma-mapping-common.h b/include/asm-generic/dma-mapping-common.h
> > > index de8bf89..d430cab 100644
> > > --- a/include/asm-generic/dma-mapping-common.h
> > > +++ b/include/asm-generic/dma-mapping-common.h
> > > @@ -16,6 +16,7 @@ static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
> > > dma_addr_t addr;
> > >
> > > kmemcheck_mark_initialized(ptr, size);
> > > + BUG_ON(!ops);
> >
> > Does this actually help anything? I expected that if "ops" is NULL,
> > we would just oops anyway when we attempted to call "ops->map_page()"
> > because we already trap null pointer dereferences. At least, when I
> > tried leaving a pci_bus.ops pointer NULL, I got a nice panic and
> > backtrace even without adding an explicit BUG_ON().
> >
> > I cc'd Marek, who added the similar BUG_ON()s in dma_mmap_attrs() and
> > dma_get_sgtable_attrs() with d2b7428eb0 and 64ccc9c033.
>
> I think that I've copied it from dma_alloc_coherent() in microblaze. You
> are right that lack
> of this check will also trigger oops in ops==NULL case, but I think that
> adding explicit check
> in all functions, which use it, is a good idea. It serves as a kind of
> documentation and
> emphasizes that missing ops is really an issue.
Really, no, it's not a good idea at all. It invites tons of patches
littering the code with BUG_ONs where we might possibly get a NULL
dereference. All it does is add extra instructions to a code path for
no actual benefit.
If you can answer the question: what more information does the BUG_ON
give you than the NULL deref Oops would not? then it might be
reasonable.
James
On Tuesday 11 June 2013, James Bottomley wrote:
> Really, no, it's not a good idea at all. It invites tons of patches
> littering the code with BUG_ONs where we might possibly get a NULL
> dereference. All it does is add extra instructions to a code path for
> no actual benefit.
>
> If you can answer the question: what more information does the BUG_ON
> give you than the NULL deref Oops would not? then it might be
> reasonable.
The question is if a user can trigger the NULL dereference intentionally,
in which case they might get the kernel to jump into a user-provided
buffer.
Arnd
On 6/12/2013 5:06 PM, Arnd Bergmann wrote:
> On Tuesday 11 June 2013, James Bottomley wrote:
> > Really, no, it's not a good idea at all. It invites tons of patches
> > littering the code with BUG_ONs where we might possibly get a NULL
> > dereference. All it does is add extra instructions to a code path for
> > no actual benefit.
> >
> > If you can answer the question: what more information does the BUG_ON
> > give you than the NULL deref Oops would not? then it might be
> > reasonable.
>
> The question is if a user can trigger the NULL dereference intentionally,
> in which case they might get the kernel to jump into a user-provided
> buffer.
I don't any possibility for userspace to alter the ops pointer, so if you
think that BUG_ON() approach causes additional overhead then I'm fine to
remove it.
Best regards
--
Marek Szyprowski
Samsung R&D Institute Poland
On Wed, 2013-06-12 at 17:06 +0200, Arnd Bergmann wrote:
> On Tuesday 11 June 2013, James Bottomley wrote:
> > Really, no, it's not a good idea at all. It invites tons of patches
> > littering the code with BUG_ONs where we might possibly get a NULL
> > dereference. All it does is add extra instructions to a code path for
> > no actual benefit.
> >
> > If you can answer the question: what more information does the BUG_ON
> > give you than the NULL deref Oops would not? then it might be
> > reasonable.
>
> The question is if a user can trigger the NULL dereference intentionally,
> in which case they might get the kernel to jump into a user-provided
> buffer.
Can you elaborate on how they could do this? If you're thinking they
could alter the pointer and trigger the jump, then yes, but a BUG_ON
won't prevent that because the altered pointer won't be NULL.
James
On Thursday 13 June 2013, James Bottomley wrote:
> On Wed, 2013-06-12 at 17:06 +0200, Arnd Bergmann wrote:
> > On Tuesday 11 June 2013, James Bottomley wrote:
> > > Really, no, it's not a good idea at all. It invites tons of patches
> > > littering the code with BUG_ONs where we might possibly get a NULL
> > > dereference. All it does is add extra instructions to a code path for
> > > no actual benefit.
> > >
> > > If you can answer the question: what more information does the BUG_ON
> > > give you than the NULL deref Oops would not? then it might be
> > > reasonable.
> >
> > The question is if a user can trigger the NULL dereference intentionally,
> > in which case they might get the kernel to jump into a user-provided
> > buffer.
>
> Can you elaborate on how they could do this? If you're thinking they
> could alter the pointer and trigger the jump, then yes, but a BUG_ON
> won't prevent that because the altered pointer won't be NULL.
The attack that has been demonstrated a couple of times uses an anomymous
mmap to virtual address 0. You fill that page with pointers to a
function in your program. If there is a NULL pointer to some operations
structure and kernel code calls an operation without checking the
ops pointer first, it gets read from the NULL page and the kernel
jumps into user space.
Arnd
On Fri, 2013-06-14 at 16:36 +0200, Arnd Bergmann wrote:
> On Thursday 13 June 2013, James Bottomley wrote:
> > On Wed, 2013-06-12 at 17:06 +0200, Arnd Bergmann wrote:
> > > On Tuesday 11 June 2013, James Bottomley wrote:
> > > > Really, no, it's not a good idea at all. It invites tons of patches
> > > > littering the code with BUG_ONs where we might possibly get a NULL
> > > > dereference. All it does is add extra instructions to a code path for
> > > > no actual benefit.
> > > >
> > > > If you can answer the question: what more information does the BUG_ON
> > > > give you than the NULL deref Oops would not? then it might be
> > > > reasonable.
> > >
> > > The question is if a user can trigger the NULL dereference intentionally,
> > > in which case they might get the kernel to jump into a user-provided
> > > buffer.
> >
> > Can you elaborate on how they could do this? If you're thinking they
> > could alter the pointer and trigger the jump, then yes, but a BUG_ON
> > won't prevent that because the altered pointer won't be NULL.
>
> The attack that has been demonstrated a couple of times uses an anomymous
> mmap to virtual address 0. You fill that page with pointers to a
> function in your program. If there is a NULL pointer to some operations
> structure and kernel code calls an operation without checking the
> ops pointer first, it gets read from the NULL page and the kernel
> jumps into user space.
This is the MMAP_PAGE_ZERO exploit. The original exploit relied on a
leaky personality capability clearing mask and was fixed in 2.6.31 by
commit f9fabcb58a6d26d6efde842d1703ac7cfa9427b6
Author: Julien Tinnes <[email protected]>
Date: Fri Jun 26 20:27:40 2009 +0200
personality: fix PER_CLEAR_ON_SETID
So it's not really relevant to 3.x kernels, is it?
James
On Friday 14 June 2013, James Bottomley wrote:
> This is the MMAP_PAGE_ZERO exploit. The original exploit relied on a
> leaky personality capability clearing mask and was fixed in 2.6.31 by
>
> commit f9fabcb58a6d26d6efde842d1703ac7cfa9427b6
> Author: Julien Tinnes <[email protected]>
> Date: Fri Jun 26 20:27:40 2009 +0200
>
> personality: fix PER_CLEAR_ON_SETID
>
> So it's not really relevant to 3.x kernels, is it?
Probably not. There is always a risk that something like this
can turn into an exploit, but it needs a combination with a couple
of other bugs.
Arnd
On 06/19/2013 05:20 PM, Arnd Bergmann wrote:
> On Friday 14 June 2013, James Bottomley wrote:
>> This is the MMAP_PAGE_ZERO exploit. The original exploit relied on a
>> leaky personality capability clearing mask and was fixed in 2.6.31 by
>>
>> commit f9fabcb58a6d26d6efde842d1703ac7cfa9427b6
>> Author: Julien Tinnes <[email protected]>
>> Date: Fri Jun 26 20:27:40 2009 +0200
>>
>> personality: fix PER_CLEAR_ON_SETID
>>
>> So it's not really relevant to 3.x kernels, is it?
>
> Probably not. There is always a risk that something like this
> can turn into an exploit, but it needs a combination with a couple
> of other bugs.
ok. Let me refresh this thread.
We have middle solution where some functions have this checking
and some not.
Based on get_maintainer scripts Arnd should do that decision
to accept or reject this patch.
Arnd: Can you please decide if you want it or not?
Based on that you can just add this one or we can create new one
which remove BUG_ON(!ops) from that file.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform