| Message ID | 20181128185910.5778-1-nathanj439@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ARM: dma-mapping: fix potential uninitialized return | expand |
On 11/28/18 6:59 PM, Nathan Jones wrote: > 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. > > Fixes: 1655cf8829d8 ("ARM: dma-mapping: Remove traces of NOMMU code") I'll leave it up to Russell. Cheers Vladimir > Signed-off-by: Nathan Jones <nathanj439@gmail.com> > --- > arch/arm/mm/dma-mapping.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index 661fe48ab78d..78de138aa66d 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -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); >
On 11/29/18 9:50 AM, Vladimir Murzin wrote: > On 11/28/18 6:59 PM, Nathan Jones wrote: >> 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 diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c index 597d408..0273ec4 100644 --- a/kernel/dma/coherent.c +++ b/kernel/dma/coherent.c @@ -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 > > I assume that it was found with static analyzer or like this, in such case > please, provide output produced by the tool. > >> >> Fixes: 1655cf8829d8 ("ARM: dma-mapping: Remove traces of NOMMU code") > > I'll leave it up to Russell. > > Cheers > Vladimir > >> Signed-off-by: Nathan Jones <nathanj439@gmail.com> >> --- >> arch/arm/mm/dma-mapping.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c >> index 661fe48ab78d..78de138aa66d 100644 >> --- a/arch/arm/mm/dma-mapping.c >> +++ b/arch/arm/mm/dma-mapping.c >> @@ -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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Thu, Nov 29, 2018 at 09:50:59AM +0000, Vladimir Murzin wrote: > On 11/28/18 6:59 PM, Nathan Jones wrote: > > 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.
On Thu, Nov 29, 2018 at 10:11:45AM +0000, Vladimir Murzin wrote: > On 11/29/18 9:50 AM, Vladimir Murzin wrote: > > On 11/28/18 6:59 PM, Nathan Jones wrote: > >> 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 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.
On Thu, Nov 29, 2018 at 4:51 AM Vladimir Murzin <vladimir.murzin@arm.com> wrote: > > On 11/28/18 6:59 PM, Nathan Jones wrote: > > 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. No, I had some bad code which was passing a wrong length and receiving the strange error code. > > > > > Fixes: 1655cf8829d8 ("ARM: dma-mapping: Remove traces of NOMMU code") > > I'll leave it up to Russell. > > Cheers > Vladimir > > > Signed-off-by: Nathan Jones <nathanj439@gmail.com> > > --- > > arch/arm/mm/dma-mapping.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > > index 661fe48ab78d..78de138aa66d 100644 > > --- a/arch/arm/mm/dma-mapping.c > > +++ b/arch/arm/mm/dma-mapping.c > > @@ -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); > > >
On 28/11/2018 18:59, Nathan Jones wrote: > 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. Reviewed-by: Robin Murphy <robin.murphy@arm.com> > Fixes: 1655cf8829d8 ("ARM: dma-mapping: Remove traces of NOMMU code") > Signed-off-by: Nathan Jones <nathanj439@gmail.com> > --- > arch/arm/mm/dma-mapping.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index 661fe48ab78d..78de138aa66d 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -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); >
On Thu, Nov 29, 2018 at 09:58:46AM -0500, Nathan Jones wrote: > 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.
On Thu, Nov 29, 2018 at 10:22:59AM +0000, Russell King - ARM Linux wrote: > 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. 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;
On 11/29/18 2:58 PM, Nathan Jones wrote: > On Thu, Nov 29, 2018 at 4:51 AM Vladimir Murzin <vladimir.murzin@arm.com> wrote: >> >> On 11/28/18 6:59 PM, Nathan Jones wrote: >>> 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. > > 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 Acked-by: Vladimir Murzin <vladimir.murzin@arm.com> Thanks Vladimir > >> >>> >>> Fixes: 1655cf8829d8 ("ARM: dma-mapping: Remove traces of NOMMU code") >> >> I'll leave it up to Russell. >> >> Cheers >> Vladimir >> >>> Signed-off-by: Nathan Jones <nathanj439@gmail.com> >>> --- >>> arch/arm/mm/dma-mapping.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c >>> index 661fe48ab78d..78de138aa66d 100644 >>> --- a/arch/arm/mm/dma-mapping.c >>> +++ b/arch/arm/mm/dma-mapping.c >>> @@ -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); >>> >> >
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 661fe48ab78d..78de138aa66d 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -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);
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 <nathanj439@gmail.com> --- arch/arm/mm/dma-mapping.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)