Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH] Staging: TIDSPBRIDGE: Use vm_iomap_memory for mmap-ing instead of remap_pfn_range

7 views
Skip to first unread message

Ivaylo DImitrov

unread,
Dec 2, 2013, 3:10:03 PM12/2/13
to
From: Ivaylo Dimitrov <freema...@abv.bg>

This fixes the following bug:

---- Bug Report ----

source file: drivers/staging/tidspbridge/rmgr/drv_interface.c
issue : mapping of physical memory without address range checks

259 static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
260 {
261 u32 status;
262
263 /* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */
264 vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
265
266 dev_dbg(bridge, "%s: vm filp %p start %lx end %lx page_prot %ulx "
267 "flags %lx\n", __func__, filp,
268 vma->vm_start, vma->vm_end, vma->vm_page_prot,
269 vma->vm_flags);
270
271 status = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
272 vma->vm_end - vma->vm_start,
273 vma->vm_page_prot);
274 if (status != 0)
275 status = -EAGAIN;
276
277 return status;
278 }

The function provides an interface to remap physical memory to user space, but
does not provide any checks to ensure that the memory is within the region that
should be accessible.

Reported-by: Nico Golde <ni...@ngolde.de>
Reported-by: Fabian Yamaguchi <fa...@goesec.de>

Signed-off-by: Ivaylo Dimitrov <freema...@abv.bg>
Signed-off-by: Pavel Machek <pa...@ucw.cz>
---
drivers/staging/tidspbridge/rmgr/drv_interface.c | 15 ++++++---------
1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 1aa4a3f..2d500ea 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -258,8 +258,9 @@ err:
/* This function maps kernel space memory to user space memory. */
static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
{
- u32 status;
-
+ struct omap_dsp_platform_data *pdata =
+ omap_dspbridge_dev->dev.platform_data;
+
/* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */
vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);

@@ -268,13 +269,9 @@ static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
vma->vm_start, vma->vm_end, vma->vm_page_prot,
vma->vm_flags);

- status = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
- vma->vm_end - vma->vm_start,
- vma->vm_page_prot);
- if (status != 0)
- status = -EAGAIN;
-
- return status;
+ return vm_iomap_memory(vma,
+ pdata->phys_mempool_base,
+ pdata->phys_mempool_size);
}

static const struct file_operations bridge_fops = {
--
1.5.6.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Steven Luo

unread,
Dec 7, 2013, 7:00:01 PM12/7/13
to
This patch causes problems with DSP codecs on OMAP3 devices running
Android -- specifically, when the decoder is cleaning up after itself,
munmap() of the mapped area fails, leading to a memory leak which
eventually crashes the system.

As far as I can tell, the code with this patch applied reduces to
(ignoring checks and such)

remap_pfn_range(vma, vma->vm_start,
(pdata->phys_mempool_base >> PAGE_SHIFT) + vma->vm_pgoff,
vma->vm_end - vma->vm_start,
vma->vm_page_prot);

whereas the original was

> - status = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> - vma->vm_end - vma->vm_start,
> - vma->vm_page_prot);

We're subtracting (pdata->phys_mempool_base >> PAGE_SHIFT) from
vma->vm_pgoff before calling vm_iomap_memory() to address the issue --
if that's satisfactory to everyone involved, I can submit the following
patch.

-Steven Luo

(please cc, not subscribed)

From: Steven Luo <ste...@steven676.net>
Date: Sat, 7 Dec 2013 02:11:20 -0800
Subject: [PATCH] tidspbridge: fix last patch to map same region of physical
memory as before

Commit 559c71fe5dc3 ("Staging: TIDSPBRIDGE: Use vm_iomap_memory for
mmap-ing instead of remap_pfn_range") had the effect of inadvertently
shifting the start of the physical memory area mapped by
pdata->phys_mempool_base. Correct this by subtracting that shift before
calling vm_iomap_memory() and adding it back afterwards.

Reported-by: Dheeraj CVR <cvr.d...@gmail.com>
Signed-off-by: Steven Luo <ste...@steven676.net>
---
drivers/staging/tidspbridge/rmgr/drv_interface.c | 29 +++++++++++++++++++---
1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 83cc3a5..d7f7d04 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -258,6 +258,9 @@ err:
/* This function maps kernel space memory to user space memory. */
static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
{
+ unsigned long base_pgoff;
+ int status;
+
struct omap_dsp_platform_data *pdata =
omap_dspbridge_dev->dev.platform_data;

@@ -269,9 +272,29 @@ static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
vma->vm_start, vma->vm_end, vma->vm_page_prot,
vma->vm_flags);

- return vm_iomap_memory(vma,
- pdata->phys_mempool_base,
- pdata->phys_mempool_size);
+ /*
+ * vm_iomap_memory() expects vma->vm_pgoff to be expressed as an offset
+ * from the start of the physical memory pool, but we're called with
+ * a pfn (physical page number) stored there instead.
+ *
+ * To avoid duplicating lots of tricky overflow checking logic,
+ * temporarily convert vma->vm_pgoff to the offset vm_iomap_memory()
+ * expects, but restore the original value once the mapping has been
+ * created.
+ */
+ base_pgoff = pdata->phys_mempool_base >> PAGE_SHIFT;
+ if (vma->vm_pgoff < base_pgoff)
+ return -EINVAL;
+ vma->vm_pgoff -= base_pgoff;
+
+ status = vm_iomap_memory(vma,
+ pdata->phys_mempool_base,
+ pdata->phys_mempool_size);
+
+ /* Restore the original value of vma->vm_pgoff */
+ vma->vm_pgoff += base_pgoff;
+
+ return status;
}

static const struct file_operations bridge_fops = {
--
1.7.10.4

Ivajlo Dimitrov

unread,
Dec 8, 2013, 8:10:01 AM12/8/13
to
Tested on Nokia N900 with Maemo 5 and Harmattan codec nodes

Ivajlo Dimitrov

unread,
Dec 11, 2013, 2:50:02 AM12/11/13
to

On 08.12.2013 01:49, Steven Luo wrote:
> This patch causes problems with DSP codecs on OMAP3 devices running
> Android -- specifically, when the decoder is cleaning up after itself,
> munmap() of the mapped area fails, leading to a memory leak which
> eventually crashes the system.
>
> As far as I can tell, the code with this patch applied reduces to
> (ignoring checks and such)
>
> remap_pfn_range(vma, vma->vm_start,
> (pdata->phys_mempool_base >> PAGE_SHIFT) + vma->vm_pgoff,
> vma->vm_end - vma->vm_start,
> vma->vm_page_prot);
>
> whereas the original was
>
>> - status = remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
>> - vma->vm_end - vma->vm_start,
>> - vma->vm_page_prot);
> We're subtracting (pdata->phys_mempool_base >> PAGE_SHIFT) from
> vma->vm_pgoff before calling vm_iomap_memory() to address the issue --
> if that's satisfactory to everyone involved, I can submit the following
> patch.
>
>
Hi,

I can pick your changes and re-send the original patch with them
incorporated if there are no objections. Are you fine with that?

Regards,
Ivo

Dan Carpenter

unread,
Dec 11, 2013, 3:40:01 AM12/11/13
to
On Wed, Dec 11, 2013 at 09:45:52AM +0200, Ivajlo Dimitrov wrote:
> I can pick your changes and re-send the original patch with them
> incorporated if there are no objections. Are you fine with that?
>

Do it on top of staging-next, don't redo the original.

regards,
dan carpenter

Ivaylo Dimitrov

unread,
Dec 11, 2013, 5:00:01 AM12/11/13
to
On 11.12.2013 10:33, Dan Carpenter wrote:
> On Wed, Dec 11, 2013 at 09:45:52AM +0200, Ivajlo Dimitrov wrote:
>> I can pick your changes and re-send the original patch with them
>> incorporated if there are no objections. Are you fine with that?
>>
> Do it on top of staging-next, don't redo the original.
>
> regards,
> dan carpenter

I don't see the original patch in the staging-next tree [0], how to
proceed? Isn't it better to resend the original patch with Steven's
changes included?

[0]
http://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/log/drivers/staging/tidspbridge?h=staging-next

Regards,
Ivo

Dan Carpenter

unread,
Dec 11, 2013, 5:30:02 AM12/11/13
to
On Wed, Dec 11, 2013 at 11:57:04AM +0200, Ivaylo Dimitrov wrote:
> On 11.12.2013 10:33, Dan Carpenter wrote:
> >On Wed, Dec 11, 2013 at 09:45:52AM +0200, Ivajlo Dimitrov wrote:
> >>I can pick your changes and re-send the original patch with them
> >>incorporated if there are no objections. Are you fine with that?
> >>
> >Do it on top of staging-next, don't redo the original.
> >
> >regards,
> >dan carpenter
>
> I don't see the original patch in the staging-next tree [0], how to
> proceed? Isn't it better to resend the original patch with Steven's
> changes included?
>
> [0] http://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/log/drivers/staging/tidspbridge?h=staging-next
>

Oops. It's in staging-linus not staging-next. I don't know how Greg
handles that tree.

regards,
dan carpenter

Ivaylo DImitrov

unread,
Dec 11, 2013, 4:10:03 PM12/11/13
to
From: Steven Luo <ste...@steven676.net>

Commit 559c71fe5dc3 ("Staging: TIDSPBRIDGE: Use vm_iomap_memory for
mmap-ing instead of remap_pfn_range") had the effect of inadvertently
shifting the start of the physical memory area mapped by
pdata->phys_mempool_base. Correct this by subtracting that shift before
calling vm_iomap_memory() and adding it back afterwards.

Reported-by: Dheeraj CVR <cvr.d...@gmail.com>
Signed-off-by: Ivaylo Dimitrov <freema...@abv.bg>
---
drivers/staging/tidspbridge/rmgr/drv_interface.c | 30 +++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 56e355b..fec5afc 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -258,6 +258,8 @@ err:
/* This function maps kernel space memory to user space memory. */
static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
{
+ unsigned long base_pgoff;
+ int status;
struct omap_dsp_platform_data *pdata =
omap_dspbridge_dev->dev.platform_data;

@@ -269,9 +271,31 @@ static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
--
1.8.4.msysgit.0

Greg KH

unread,
Dec 11, 2013, 9:00:01 PM12/11/13
to
On Wed, Dec 11, 2013 at 01:27:17PM +0300, Dan Carpenter wrote:
> On Wed, Dec 11, 2013 at 11:57:04AM +0200, Ivaylo Dimitrov wrote:
> > On 11.12.2013 10:33, Dan Carpenter wrote:
> > >On Wed, Dec 11, 2013 at 09:45:52AM +0200, Ivajlo Dimitrov wrote:
> > >>I can pick your changes and re-send the original patch with them
> > >>incorporated if there are no objections. Are you fine with that?
> > >>
> > >Do it on top of staging-next, don't redo the original.
> > >
> > >regards,
> > >dan carpenter
> >
> > I don't see the original patch in the staging-next tree [0], how to
> > proceed? Isn't it better to resend the original patch with Steven's
> > changes included?
> >
> > [0] http://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/log/drivers/staging/tidspbridge?h=staging-next
> >
>
> Oops. It's in staging-linus not staging-next. I don't know how Greg
> handles that tree.

The same way I do my others:
*-next : for the "next" kernel merge window
*-linus : for Linus's tree now before the -final release comes out.

The original patch here went to Linus, so it was in staging-linus and
it's already in Linus's tree.

thanks,

greg k-h
0 new messages