[PATCH] mm:bugfix check return value of ioremap_prot

[PATCH] mm:bugfix check return value of ioremap_prot

[chenjie6]

From: chen jie <chen jie@chenjie6@huwei.com>

	ioremap_prot can return NULL which could lead to an oops

Signed-off-by: chen jie <chenjie6@huawei.com>
---
 mm/memory.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/memory.c b/mm/memory.c
index 7206a63..316c42e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4397,6 +4397,9 @@ int generic_access_phys(struct vm_area_struct *vma, unsigned long addr,
 		return -EINVAL;
 
 	maddr = ioremap_prot(phys_addr, PAGE_ALIGN(len + offset), prot);
+	if (!maddr)
+		return -ENOMEM;
+
 	if (write)
 		memcpy_toio(maddr + offset, buf, len);
 	else
-- 
1.8.3.4

Re: [PATCH] mm:bugfix check return value of ioremap_prot

[Yang Shi]

On Thu, Aug 2, 2018 at 12:37 AM,  <chenjie6@huawei.com> wrote:
> From: chen jie <chen jie@chenjie6@huwei.com>
>
>         ioremap_prot can return NULL which could lead to an oops

What oops? You'd better to have the oops information in your commit log.

Thanks,
Yang

>
> Signed-off-by: chen jie <chenjie6@huawei.com>
> ---
>  mm/memory.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 7206a63..316c42e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4397,6 +4397,9 @@ int generic_access_phys(struct vm_area_struct *vma, unsigned long addr,
>                 return -EINVAL;
>
>         maddr = ioremap_prot(phys_addr, PAGE_ALIGN(len + offset), prot);
> +       if (!maddr)
> +               return -ENOMEM;
> +
>         if (write)
>                 memcpy_toio(maddr + offset, buf, len);
>         else
> --
> 1.8.3.4
>

Re: [PATCH] mm:bugfix check return value of ioremap_prot

[Andrew Morton]

On Thu, 2 Aug 2018 09:47:52 -0700 Yang Shi <shy828301@gmail.com> wrote:

> On Thu, Aug 2, 2018 at 12:37 AM,  <chenjie6@huawei.com> wrote:
> > From: chen jie <chen jie@chenjie6@huwei.com>
> >
> >         ioremap_prot can return NULL which could lead to an oops
> 
> What oops? You'd better to have the oops information in your commit log.

Doesn't matter much - the code is clearly buggy.

Looking at the callers, I have suspicions about
fs/proc/base.c:environ_read().  It's assuming that access_remote_vm()
returns an errno.  But it doesn't - it returns number of bytes copied.

Alexey, could you please take a look?  While in there, I'd suggest
adding some return value documentation to __access_remote_vm() and
access_remote_vm().  Thanks.

Re: [PATCH] mm:bugfix check return value of ioremap_prot

[Alexey Dobriyan]

On Thu, Aug 02, 2018 at 02:02:22PM -0700, Andrew Morton wrote:
> On Thu, 2 Aug 2018 09:47:52 -0700 Yang Shi <shy828301@gmail.com> wrote:
> 
> > On Thu, Aug 2, 2018 at 12:37 AM,  <chenjie6@huawei.com> wrote:
> > > From: chen jie <chen jie@chenjie6@huwei.com>
> > >
> > >         ioremap_prot can return NULL which could lead to an oops
> > 
> > What oops? You'd better to have the oops information in your commit log.
> 
> Doesn't matter much - the code is clearly buggy.
> 
> Looking at the callers, I have suspicions about
> fs/proc/base.c:environ_read().  It's assuming that access_remote_vm()
> returns an errno.  But it doesn't - it returns number of bytes copied.
> 
> Alexey, could you please take a look?  While in there, I'd suggest
> adding some return value documentation to __access_remote_vm() and
> access_remote_vm().  Thanks.

This is true: remote VM accessors return number of bytes copied
but ->access returns len/-E. Returning "int" is deceptive.

Re: [PATCH] mm:bugfix check return value of ioremap_prot

[Andrew Morton]

On Fri, 3 Aug 2018 15:47:01 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Thu, Aug 02, 2018 at 02:02:22PM -0700, Andrew Morton wrote:
> > On Thu, 2 Aug 2018 09:47:52 -0700 Yang Shi <shy828301@gmail.com> wrote:
> > 
> > > On Thu, Aug 2, 2018 at 12:37 AM,  <chenjie6@huawei.com> wrote:
> > > > From: chen jie <chen jie@chenjie6@huwei.com>
> > > >
> > > >         ioremap_prot can return NULL which could lead to an oops
> > > 
> > > What oops? You'd better to have the oops information in your commit log.
> > 
> > Doesn't matter much - the code is clearly buggy.
> > 
> > Looking at the callers, I have suspicions about
> > fs/proc/base.c:environ_read().  It's assuming that access_remote_vm()
> > returns an errno.  But it doesn't - it returns number of bytes copied.
> > 
> > Alexey, could you please take a look?  While in there, I'd suggest
> > adding some return value documentation to __access_remote_vm() and
> > access_remote_vm().  Thanks.
> 
> This is true: remote VM accessors return number of bytes copied
> but ->access returns len/-E. Returning "int" is deceptive.

It's more than deceptive - it's flat-out buggy for >4G copy attempts. 
And highly dubious for 2G-4G copies, where it might return a negative
int.

I suppose that access_remote_vm() should strictly return a ptrdiff_t,
but we hardly ever use that.  size_t will do.