ARM: dma-mapping: fix potential uninitialized return
Subsystems (2)
PATCH 1/1 ARM: dma-mapping: fix potential uninitialized return
If neither of the if() statements fire then the return value is uninitialized. In the worst case it returns 0 which means the caller will think the function succeeded.
--- arch/arm/mm/dma-mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/dma-mapping.c +1 -1
@@ -829,7 +829,7 @@ static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs) { - int ret; + int ret = -ENXIO;unsigned long nr_vma_pages =vma_pages (vma);unsigned long nr_pages =PAGE_ALIGN (size) >>PAGE_SHIFT ;unsigned long pfn =dma_to_pfn (dev, dma_addr);
Signature · 1 line
2.19.1
Russell King - ARM Linux
maintainerPATCH 1/1 ARM: dma-mapping: fix potential uninitialized return
Quoted 5 lines · Nathan Jones
If neither of the
if()statements fire then the return value is uninitialized. In the worst case it returns 0 which means the caller will think the function succeeded.Signed-off-by Nathan Jones <[email protected]>
This needs a fixes tag, since this bug was introduced by:
commit 1655cf8829d82d367d8fdb5cb58e5885d7d2a391
Author: Vladimir Murzin <[email protected]> Date: Wed May 24 11:24:32 2017 +0100
ARM: dma-mapping: Remove traces of NOMMU code
DMA operations for NOMMU case have been just factored out into separate compilation unit, so don't keep dead code.
Quoted 25 lines
--- arch/arm/mm/dma-mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/mm/dma-mapping.c +1
-1@@ -829,7 +829,7 @@ static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs) { - int ret; + int ret = -ENXIO;unsigned long nr_vma_pages =vma_pages (vma);unsigned long nr_pages =PAGE_ALIGN (size) >>PAGE_SHIFT ;unsigned long pfn =dma_to_pfn (dev, dma_addr);Signature · 7 lines
2.19.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
PATCH v2 1/1 ARM: dma-mapping: fix potential uninitialized return
In reply to Nathan JonesIf neither of the if() statements fire then the return value is uninitialized. In the worst case it returns 0 which means the caller will think the function succeeded.
--- arch/arm/mm/dma-mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/dma-mapping.c +1 -1
@@ -829,7 +829,7 @@ static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs) { - int ret; + int ret = -ENXIO;unsigned long nr_vma_pages =vma_pages (vma);unsigned long nr_pages =PAGE_ALIGN (size) >>PAGE_SHIFT ;unsigned long pfn =dma_to_pfn (dev, dma_addr);
Signature · 1 line
2.19.1
PATCH v2 1/1 ARM: dma-mapping: fix potential uninitialized return
Quoted 3 lines · Nathan Jones
If neither of the
if()statements fire then the return value is uninitialized. In the worst case it returns 0 which means the caller will think the function succeeded.
"ret" is updated indirectly via:
if (dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
return ret;
I assume that it was found with static analyzer or like this, in such case please, provide output produced by the tool.
Quoted 2 lines
Fixes 1655cf8829d8 ("ARM: dma-mapping: Remove traces of NOMMU code")
I'll leave it up to Russell.
Cheers
Vladimir
Quoted 19 lines
Signed-off-by Nathan Jones <[email protected]>--- arch/arm/mm/dma-mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/mm/dma-mapping.c +1
-1@@ -829,7 +829,7 @@ static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs) { - int ret; + int ret = -ENXIO;unsigned long nr_vma_pages =vma_pages (vma);unsigned long nr_pages =PAGE_ALIGN (size) >>PAGE_SHIFT ;unsigned long pfn =dma_to_pfn (dev, dma_addr);
PATCH v2 1/1 ARM: dma-mapping: fix potential uninitialized return
Quoted 9 lines · Vladimir Murzin
Quoted 3 lines · Nathan Jones
If neither of the
if()statements fire then the return value is uninitialized. In the worst case it returns 0 which means the caller will think the function succeeded."ret" is updated indirectly via:
if (
dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
return ret;
Ok. I've had a look how dma_mmap_from_dev_coherent() is implemented and it looks like "ret" is not updated if dev doesn't have reserved memory. It looks like arm64 also might be affected by this as well.
So, would it be better to update dma_mmap_from_dev_coherent() with
kernel/dma/coherent.c +2 -1
@@ -282,6 +282,8 @@ int dma_release_from_global_coherent(int order, void *vaddr)static int __dma_mmap_from_coherent (struct dma_coherent_mem *mem,struct vm_area_struct *vma,void *vaddr,size_t size,int *ret) { + *ret = -ENXIO ; +if (mem && vaddr >= mem->virt_base && vaddr + size <= (mem->virt_base + (mem->size <<PAGE_SHIFT ))) {unsigned long off = vma->vm_pgoff;
@@ -289,7 +291,6 @@ static int __dma_mmap_from_coherent(struct dma_coherent_mem *mem,int user_count =vma_pages (vma);int count =PAGE_ALIGN (size) >>PAGE_SHIFT ; - *ret = -ENXIO ;if (off < count && user_count <= count - off) {unsigned long pfn = mem->pfn_base + start + off; *ret = remap_pfn_range(vma, vma->vm_start, pfn,
Cheers
Vladimir
Quoted 38 lines
I assume that it was found with static analyzer or like this, in such case please, provide output produced by the tool.
Quoted 2 lines
Fixes 1655cf8829d8 ("ARM: dma-mapping: Remove traces of NOMMU code")I'll leave it up to Russell.
Cheers
VladimirQuoted 11 lines
Signed-off-by Nathan Jones <[email protected]>--- arch/arm/mm/dma-mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/mm/dma-mapping.c +0
-0arch/arm/mm/dma-mapping.c static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, · line 835
Quoted 8 lines
arch/arm/mm/dma-mapping.c +1
-1@@ -829,7 +829,7 @@ static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs) { - int ret; + int ret = -ENXIO;unsigned long nr_vma_pages =vma_pages (vma);unsigned long nr_pages =PAGE_ALIGN (size) >>PAGE_SHIFT ;unsigned long pfn =dma_to_pfn (dev, dma_addr);_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Russell King - ARM Linux
maintainerPATCH v2 1/1 ARM: dma-mapping: fix potential uninitialized return
Quoted 8 lines · Vladimir Murzin
Quoted 7 lines · Vladimir Murzin
Quoted 3 lines · Nathan Jones
If neither of the
if()statements fire then the return value is uninitialized. In the worst case it returns 0 which means the caller will think the function succeeded."ret" is updated indirectly via:
Quoted 8 lines
Quoted 2 lines
if (
dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
return ret;Ok. I've had a look how
dma_mmap_from_dev_coherent()is implemented and it looks like "ret" is not updated if dev doesn't have reserved memory. It looks like arm64 also might be affected by this as well.So, would it be better to update
dma_mmap_from_dev_coherent()with
I don't think so - if we were to remove the call to dma_mmap_from_dev_coherent(), it reintroduces the bug.
Quite why we have dma_mmap_from_dev_coherent() returning a 0/1 and error code via pointer I'm really not sure.
ret = dma_mmap_from_dev_coherent(...); if (ret) return ret > 0 ? 0 : ret;
and have dma_mmap_from_dev_coherent() return -ve for errnos, 1 if mapped via the coherent pool mechanism, or 0 otherwise.
The down-side is that 'ret' would be zero for the follow-on code, which would need explicit initialisation - but at least it's then obvious what is going on.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Quoted 10 lines · Russell King - ARM Linux
I don't think so - if we were to remove the call to
dma_mmap_from_dev_coherent(), it reintroduces the bug.Quite why we have
dma_mmap_from_dev_coherent()returning a 0/1 and error code via pointer I'm really not sure.ret = dma_mmap_from_dev_coherent(...); if (ret) return ret > 0 ? 0 : ret;
Quoted 6 lines
and have
dma_mmap_from_dev_coherent()return -ve for errnos, 1 if mapped via the coherent pool mechanism, or 0 otherwise.The down-side is that 'ret' would be zero for the follow-on code, which would need explicit initialisation - but at least it's then obvious what is going on.
The above would be better than the current calling conventions, which are horrible. But the magic positive error code also tends to lead to subtle errors sometimes. My preference for this pattern is something like:
bool mapped; ret = dma_mmap_from_dev_coherent(..., &mapped); if (ret || mapped) return ret;
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Russell King - ARM Linux
maintainerPATCH v2 1/1 ARM: dma-mapping: fix potential uninitialized return
In reply to Vladimir MurzinQuoted 9 lines · Vladimir Murzin
Quoted 3 lines · Nathan Jones
If neither of the
if()statements fire then the return value is uninitialized. In the worst case it returns 0 which means the caller will think the function succeeded."ret" is updated indirectly via:
if (
dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
return ret;
I'm afraid you are wrong - "ret" really is used uninitialised.
dma_mmap_from_dev_coherent() calls __dma_mmap_from_coherent(), and there is a top-function-level if statement:
static int __dma_mmap_from_coherent(struct dma_coherent_mem *mem,
struct vm_area_struct *vma, void *vaddr, size_t size, int *ret)
{
if (mem && vaddr >= mem->virt_base && vaddr + size <=
(mem->virt_base + (mem->size << PAGE_SHIFT))) {
... path that sets *ret ...
return 1;
}
return 0;
}If that if statement is true, then yes, ret will be initialised, and dma_mmap_from_dev_coherent() will return 1. We will take that "return ret" statement in __arm_dma_mmap() and everything is fine.
If that if statement is false, then the function returns zero without setting "ret" to anything, and we continue processing __arm_dma_mmap(). The next statements are:
if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) {
ret = remap_pfn_range(vma, vma->vm_start,
pfn + off,
vma->vm_end - vma->vm_start,
vma->vm_page_prot);
}
return ret;So, if _this_ if statement is also false, then we get to "return ret" but ret has not been initialised by anything.
Therefore, 'ret' does need initialisation, since as of your commit, there is a path through the code where it is used uninitialised.
Hence, Nathan's patch is correct.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Quoted 6 lines · Vladimir Murzin
Quoted 3 lines · Nathan Jones
If neither of the
if()statements fire then the return value is uninitialized. In the worst case it returns 0 which means the caller will think the function succeeded.
Quoted 7 lines
"ret" is updated indirectly via:
if (
dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
return ret;I assume that it was found with static analyzer or like this, in such case please, provide output produced by the tool.
No, I had some bad code which was passing a wrong length and receiving the strange error code.
Quoted 14 lines
Quoted 3 lines
Fixes 1655cf8829d8 ("ARM: dma-mapping: Remove traces of NOMMU code")I'll leave it up to Russell.
Cheers
VladimirQuoted 5 lines
Signed-off-by Nathan Jones <[email protected]>--- arch/arm/mm/dma-mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines
Quoted 6 lines
arch/arm/mm/dma-mapping.c +0
-0arch/arm/mm/dma-mapping.c static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, · line 835
Quoted 8 lines
arch/arm/mm/dma-mapping.c +1
-1@@ -829,7 +829,7 @@ static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs) { - int ret; + int ret = -ENXIO;unsigned long nr_vma_pages =vma_pages (vma);unsigned long nr_pages =PAGE_ALIGN (size) >>PAGE_SHIFT ;unsigned long pfn =dma_to_pfn (dev, dma_addr);
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Russell King - ARM Linux
maintainerQuoted 2 lines · Nathan Jones
No, I had some bad code which was passing a wrong length and receiving the strange error code.
While adding some networking documentation, I tripped over the comments at the bottom of Documentation/networking/netdev-FAQ.rst which seem very good at guiding what should be in the commit message, specifically:
If your change is a bug fix, make sure your commit log indicates the end-user visible symptom, the underlying reason as to why it happens, and then if necessary, explain why the fix proposed is the best way to get things done.
This is actually rather important, as we may need to look back at a commit, and end up wondering what it was about. A case in point is the patch that added the /proc/<PID>/syscall interface:
/proc/PID/syscall
This adds /proc/PID/syscall and /proc/PID/task/TID/syscall magic files. These use task_current_syscall() to show the task's current system call number and argument registers, stack pointer and PC. For a task blocked but not in a syscall, the file shows "-1" in place of the syscall number, followed by only the SP and PC. For a task that's not blocked, it shows "running".
This doesn't say what the purpose of this new user interface is, why it is needed, nor is there documentation describing its behaviour (such as what happens if the thread is being traced.) The covering message for the series omitted to talk about this new interface. So we're now at the position where we have a bug reported against this interface, and no one knows what the right behaviour is really supposed to be.
The commit message describes mostly what we can gather from reading the patch, and some of the behaviour but is entirely insufficient - we're left scratching our heads as to what the behaviour should be for restarted syscalls.
So, augmenting your commit message with something like:
"While trying to use the dma_mmap_*() interface, it was noticed that
this interface returns strange values when passed an incorrect length."
would be nice.
Thanks.
Signature · 8 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Vladimir Murzin
Acked-byQuoted 9 lines · Nathan Jones
Quoted 8 lines · Vladimir Murzin
Quoted 3 lines · Nathan Jones
If neither of the
if()statements fire then the return value is uninitialized. In the worst case it returns 0 which means the caller will think the function succeeded."ret" is updated indirectly via:
Quoted 8 lines
Quoted 5 lines
if (
dma_mmap_from_dev_coherent(dev, vma, cpu_addr, size, &ret))
return ret;I assume that it was found with static analyzer or like this, in such case please, provide output produced by the tool.
No, I had some bad code which was passing a wrong length and receiving the strange error code.
Sorry for dragging you into that. If you put statement above into commit log, feel free to add
Thanks
Vladimir
Quoted 15 lines
Quoted 15 lines
Quoted 4 lines
Fixes 1655cf8829d8 ("ARM: dma-mapping: Remove traces of NOMMU code")I'll leave it up to Russell.
Cheers
VladimirQuoted 5 lines
Signed-off-by Nathan Jones <[email protected]>--- arch/arm/mm/dma-mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 16 lines
Quoted 6 lines
Quoted 6 lines
arch/arm/mm/dma-mapping.c +0
-0@@ -829,7 +829,7 @@ static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size,Quoted 9 lines
arch/arm/mm/dma-mapping.c static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, · line 835
Quoted 8 lines
arch/arm/mm/dma-mapping.c +1
-1unsigned long attrs) { -int ret; +int ret = -ENXIO ;unsigned long nr_vma_pages =vma_pages (vma);unsigned long nr_pages =PAGE_ALIGN (size) >>PAGE_SHIFT ;unsigned long pfn =dma_to_pfn (dev, dma_addr);
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 3 lines · Nathan Jones
If neither of the
if()statements fire then the return value is uninitialized. In the worst case it returns 0 which means the caller will think the function succeeded.
Looking at the referenced commit, the original initialisation of ret must have actually belonged to the MMU code anyway, since all the NOMMU path did was overwrite it unconditionally. Thus the patch itself and the fixes tag both look like the right thing to do.
Quoted 12 lines
Fixes 1655cf8829d8 ("ARM: dma-mapping: Remove traces of NOMMU code")Signed-off-by Nathan Jones <[email protected]>--- arch/arm/mm/dma-mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/mm/dma-mapping.c +0
-0
arch/arm/mm/dma-mapping.c static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, · line 835
Quoted 8 lines
arch/arm/mm/dma-mapping.c +1
-1@@ -829,7 +829,7 @@ static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs) { - int ret; + int ret = -ENXIO;unsigned long nr_vma_pages =vma_pages (vma);unsigned long nr_pages =PAGE_ALIGN (size) >>PAGE_SHIFT ;unsigned long pfn =dma_to_pfn (dev, dma_addr);
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v3 1/1 ARM: dma-mapping: fix potential uninitialized return
In reply to Nathan JonesWhile trying to use the dma_mmap_*() interface, it was noticed that this interface returns strange values when passed an incorrect length.
If neither of the if() statements fire then the return value is uninitialized. In the worst case it returns 0 which means the caller will think the function succeeded.
--- arch/arm/mm/dma-mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/dma-mapping.c +1 -1
@@ -829,7 +829,7 @@ static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs) { - int ret; + int ret = -ENXIO;unsigned long nr_vma_pages =vma_pages (vma);unsigned long nr_pages =PAGE_ALIGN (size) >>PAGE_SHIFT ;unsigned long pfn =dma_to_pfn (dev, dma_addr);
Signature · 7 lines
2.19.1 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 20 lines · Nathan Jones
While trying to use the dma_mmap_*() interface, it was noticed that this interface returns strange values when passed an incorrect length.
If neither of the
if()statements fire then the return value is uninitialized. In the worst case it returns 0 which means the caller will think the function succeeded.Fixes 1655cf8829d8 ("ARM: dma-mapping: Remove traces of NOMMU code")Signed-off-by Nathan Jones <[email protected]>Acked-by Vladimir Murzin <[email protected]>--- arch/arm/mm/dma-mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/mm/dma-mapping.c +0
-0
arch/arm/mm/dma-mapping.c static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, · line 835
Quoted 8 lines
arch/arm/mm/dma-mapping.c +1
-1@@ -829,7 +829,7 @@ static int __arm_dma_mmap(struct device *dev, struct vm_area_struct *vma, void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs) { - int ret; + int ret = -ENXIO;unsigned long nr_vma_pages =vma_pages (vma);unsigned long nr_pages =PAGE_ALIGN (size) >>PAGE_SHIFT ;unsigned long pfn =dma_to_pfn (dev, dma_addr);
FYI, it is now in Russell's tracker [1] as patch 8816/1
[1] http://www.armlinux.org.uk/developer/patches/
Thanks
Vladimir
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel