| Message ID | 1533195441-58594-1-git-send-email-chenjie6@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm:bugfix check return value of ioremap_prot | expand |
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 >
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.
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.
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.
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