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

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

[Ivaylo DImitrov]

From: Ivaylo Dimitrov <freemangordon@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 <nico@ngolde.de>
Reported-by: Fabian Yamaguchi <fabs@goesec.de>

Signed-off-by: Ivaylo Dimitrov <freemangordon@abv.bg>
Signed-off-by: Pavel Machek <pavel@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

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

[Steven Luo]

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 <steven@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.dheeraj@gmail.com>
Signed-off-by: Steven Luo <steven@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

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

[Ivajlo Dimitrov]

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.
>
> -Steven Luo
>
> (please cc, not subscribed)
>
> From: Steven Luo <steven@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.dheeraj@gmail.com>
> Signed-off-by: Steven Luo <steven@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 = {
Tested on Nokia N900 with Maemo 5 and Harmattan codec nodes

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

[Ivajlo Dimitrov]

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

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

[Dan Carpenter]

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

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

[Ivaylo Dimitrov]

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

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

[Dan Carpenter]

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

[PATCH] Staging: TIDSPBRIDGE: Fix mmap to map the correct region of physical memory

[Ivaylo DImitrov]

From: Steven Luo <steven@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.dheeraj@gmail.com>
Signed-off-by: Ivaylo Dimitrov <freemangordon@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)
 		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.8.4.msysgit.0

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

[Greg KH]

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