[PATCH] ARM: dma-mapping: fix potential uninitialized return

[PATCH] ARM: dma-mapping: fix potential uninitialized return

[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 <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);
-- 
2.19.1

[PATCH] ARM: dma-mapping: fix potential uninitialized return

[Russell King - ARM Linux]

On Wed, Nov 28, 2018 at 12:39:29PM -0500, 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.
> 
> Signed-off-by: Nathan Jones <nathanj439@gmail.com>

This needs a fixes tag, since this bug was introduced by:

commit 1655cf8829d82d367d8fdb5cb58e5885d7d2a391
Author: Vladimir Murzin <vladimir.murzin@arm.com>
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.

    Tested-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
    Tested-by: Andras Szemzo <sza@esh.hu>
    Tested-by: Alexandre TORGUE <alexandre.torgue@st.com>
    Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
    Acked-by: Arnd Bergmann <arnd@arndb.de>
    Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
    Signed-off-by: Christoph Hellwig <hch@lst.de>

> ---
>  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);
> -- 
> 2.19.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
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] ARM: dma-mapping: fix potential uninitialized return

[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.

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);
-- 
2.19.1

[PATCH v2] ARM: dma-mapping: fix potential uninitialized return

[Vladimir Murzin]

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);
> 

[PATCH v2] ARM: dma-mapping: fix potential uninitialized return

[Vladimir Murzin]

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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH v2] ARM: dma-mapping: fix potential uninitialized return

[Russell King - ARM Linux]

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.

-- 
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] ARM: dma-mapping: fix potential uninitialized return

[Russell King - ARM Linux]

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.

-- 
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

Re: [PATCH v2] ARM: dma-mapping: fix potential uninitialized return

[Nathan Jones]

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);
> >
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] ARM: dma-mapping: fix potential uninitialized return

[Robin Murphy]

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);
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] ARM: dma-mapping: fix potential uninitialized return

[Russell King - ARM Linux]

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.

-- 
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
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] ARM: dma-mapping: fix potential uninitialized return

[Christoph Hellwig]

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;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] ARM: dma-mapping: fix potential uninitialized return

[Vladimir Murzin]

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);
>>>
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3] ARM: dma-mapping: fix potential uninitialized return

[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 <nathanj439@gmail.com>
Acked-by: Vladimir Murzin <vladimir.murzin@arm.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);
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] ARM: dma-mapping: fix potential uninitialized return

[Vladimir Murzin]

On 11/30/18 1:07 PM, Nathan Jones wrote:
> 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 <nathanj439@gmail.com>
> Acked-by: Vladimir Murzin <vladimir.murzin@arm.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);
> 

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
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel