Staging: tidspbridge: disable driver

Staging: tidspbridge: disable driver

[Ivajlo Dimitrov]

Hi,

(re-sending in plain text, sorry for the noise)

commit 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=930ba4a374b96560ef9fde2145cdc454a164ddcc
disables tidspbridge driver for the reasons it has a security bug and there 
is no active maintainer.
I was following the development of that driver for a bit and my impression 
was that the reason it is
still in staging is this 
http://www.spinics.net/lists/linux-omap/msg84115.html . And as dmtimer
driver is on its way to be merged upstream(or was it already merged?)  I was 
planning to work on
tidspbridge driver to fix it and send the patches. I also have a couple of 
another small patches
ready to be send.

However, could you elaborate on the "security bug" so I can try to fix it 
and send the patch?
Also, what needs to be done for the tidspdriver to get out of staging as it 
seems that what I
though initially is incorrect.

Regards,
Ivo

PS: my further mails will be from this account on gmal as it seems LKML does 
not like
my abv.bg account (freemangordon at abv dot bg) 

Re: Staging: tidspbridge: disable driver

[Greg KH]

On Sat, Nov 30, 2013 at 11:58:23AM +0200, Ivajlo Dimitrov wrote:
> Hi,
> 
> (re-sending in plain text, sorry for the noise)
> 
> commit 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=930ba4a374b96560ef9fde2145cdc454a164ddcc
> disables tidspbridge driver for the reasons it has a security bug and there 
> is no active maintainer.
> I was following the development of that driver for a bit and my impression 
> was that the reason it is
> still in staging is this 
> http://www.spinics.net/lists/linux-omap/msg84115.html . And as dmtimer
> driver is on its way to be merged upstream(or was it already merged?)  I was 
> planning to work on
> tidspbridge driver to fix it and send the patches. I also have a couple of 
> another small patches
> ready to be send.
> However, could you elaborate on the "security bug" so I can try to fix it 
> and send the patch?
> Also, what needs to be done for the tidspdriver to get out of staging as it 
> seems that what I
> though initially is incorrect.

There is also the very long TODO list in drivers/staging/tidspbridge/

What is the progression on that?

thanks,

greg k-h

Re: Staging: tidspbridge: disable driver

[Tony Lindgren]

* Greg KH <gregkh@linuxfoundation.org> [131130 08:20]:
> On Sat, Nov 30, 2013 at 11:58:23AM +0200, Ivajlo Dimitrov wrote:
> > Hi,
> > 
> > (re-sending in plain text, sorry for the noise)
> > 
> > commit 
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=930ba4a374b96560ef9fde2145cdc454a164ddcc
> > disables tidspbridge driver for the reasons it has a security bug and there 
> > is no active maintainer.
> > I was following the development of that driver for a bit and my impression 
> > was that the reason it is
> > still in staging is this 
> > http://www.spinics.net/lists/linux-omap/msg84115.html . And as dmtimer
> > driver is on its way to be merged upstream(or was it already merged?)  I was 
> > planning to work on
> > tidspbridge driver to fix it and send the patches. I also have a couple of 
> > another small patches
> > ready to be send.

I'd rather not just export 27 omap specific functions for the dmtimer to the
drivers knowing what kind of mess it will be.. There's some work happening
to make the dmtimer more generic, so for those parts we should have a solution
soonish.

> > However, could you elaborate on the "security bug" so I can try to fix it 
> > and send the patch?
> > Also, what needs to be done for the tidspdriver to get out of staging as it 
> > seems that what I
> > though initially is incorrect.
> 
> There is also the very long TODO list in drivers/staging/tidspbridge/
> 
> What is the progression on that?

Yeah and the tidspbridge should use the remoteproc which seems to be missing
from the TODO file. Anyways, the pending dmtimer work should not impact fixing
the other issues from development point of view.

Regards,

Tony

Re: Staging: tidspbridge: disable driver

[Pavel Machek]

Hi!

> However, could you elaborate on the "security bug" so I can try to
> fix it and send the patch?
> Also, what needs to be done for the tidspdriver to get out of
> staging as it seems that what I
> though initially is incorrect.

Commit

commit 930ba4a374b96560ef9fde2145cdc454a164ddcc
Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date:   Wed Nov 27 09:32:49 2013 -0800

    Staging: tidspbridge: disable driver
    
    There seems to be no active maintainer for the driver, and there
    is an
    unfixed security bug, so disable the driver for now.
    
    Hopefully someone steps up to be the maintainer, and works to get
    this
    out of staging, otherwise it will be deleted soon.
    
    Reported-by: Nico Golde <nico@ngolde.de>
    Reported-by: Dan Carpenter <dan.carpenter@oracle.com>


so perhaps Nico Golde or Dan Carpenter can elaborate? I Cc-ed them
now.

Or is it some kind of super-secret issue and still under embargo for
10 days?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Staging: tidspbridge: disable driver

[Dan Carpenter]

On Sat, Nov 30, 2013 at 08:19:32PM +0100, Pavel Machek wrote:
> so perhaps Nico Golde or Dan Carpenter can elaborate? I Cc-ed them
> now.
> 
> Or is it some kind of super-secret issue and still under embargo for
> 10 days?
> 									Pavel

Nope,  it's not secret.  Here is the original report from Nico and
Fabian.  Please give them credit when you fix the bug.

Reported-by: Nico Golde <nico@ngolde.de>
Reported-by: Fabian Yamaguchi <fabs@goesec.de>

Felipe Contreras says we could just remove mmap() support and the driver
would still work, but no one has submitted a patch to do that.  Really
this driver needs an actual maintainer who will respond to security bugs
and also start to move the driver out of staging.

regards,
dan carpenter

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

[patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Pavel Machek]

mmap in tidspbridge is missing range-checks. For now, make this
interface root-only, so that it does not cause security problems.

Signed-off-by: Pavel Machek <pavel@ucw.cz>
Reported-by: Nico Golde <nico@ngolde.de>
Reported-by: Fabian Yamaguchi <fabs@goesec.de>

---

On Sat 2013-11-30 22:49:35, Dan Carpenter wrote:
> On Sat, Nov 30, 2013 at 08:19:32PM +0100, Pavel Machek wrote:
> > so perhaps Nico Golde or Dan Carpenter can elaborate? I Cc-ed them
> > now.
> > 
> > Or is it some kind of super-secret issue and still under embargo for
> > 10 days?
> 
> Nope,  it's not secret.  Here is the original report from Nico and
> Fabian.  Please give them credit when you fix the bug.

Thanks!

> Felipe Contreras says we could just remove mmap() support and the driver
> would still work, but no one has submitted a patch to do that.  Really
> this driver needs an actual maintainer who will respond to security bugs
> and also start to move the driver out of staging.

There starts to be real traction around Nokia N900, so I believe it is
going to be fixed. (And with Nemo/Mer getting popularity due to Jolla,
it should get even better).

But no, I did not figure out how to run Nemo in qemu, yet...
									Pavel 

diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 1aa4a3f..8533e67 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -258,7 +258,10 @@ 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;
+	int status;
+
+	if (!capable(CAP_SYS_RAWIO))
+		return -EPERM;
 
 	/* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
@@ -271,10 +274,10 @@ static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
 	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;
+	if (status)
+		return -EAGAIN;
 
-	return status;
+	return 0;
 }
 
 static const struct file_operations bridge_fops = {

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Greg KH]

On Sat, Nov 30, 2013 at 09:42:37PM +0100, Pavel Machek wrote:
> 
> mmap in tidspbridge is missing range-checks. For now, make this
> interface root-only, so that it does not cause security problems.

Please fix this properly and don't paper over the real problem here.

thanks,

greg k-h

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Pavel Machek]

On Sat 2013-11-30 14:05:53, Greg KH wrote:
> On Sat, Nov 30, 2013 at 09:42:37PM +0100, Pavel Machek wrote:
> > 
> > mmap in tidspbridge is missing range-checks. For now, make this
> > interface root-only, so that it does not cause security problems.
> 
> Please fix this properly and don't paper over the real problem here.

Well, if the driver is left uncompilable, I'm pretty sure it will
bitrot.

I do have the hardware, but I'm pretty sure current mailine does not
have enough support to boot Maemo, so it is non trivial for me to test
changes here.

And yes, I'd like to get N900 to better shape, but there's more urgent
work to do there. Such as "make sure N900 still boots once omap moves away
from device files".

[It seems like check should be that

vma->vm_pgoff << PAGE_SHIFT >= pdata->phys_mempool_base and 
vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT - pdata->phys_mempool_base) <= pdata->phys_mempool_size .

But... this is some kind of DSP coprocessor, and I am not sure if
just exposing its address space to untrusted processes is good idea.

Heck, are you sure this is security problem in the first place? Yes,
it is unchecked mmap. So what? If the device is 600 root.root, and if
the DSP can take over main system, 

        if (!capable(CAP_SYS_RAWIO))
                return -EPERM;

is exactly right solution, it just should be in _open...]

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Greg KH]

On Sat, Nov 30, 2013 at 11:58:23PM +0100, Pavel Machek wrote:
> On Sat 2013-11-30 14:05:53, Greg KH wrote:
> > On Sat, Nov 30, 2013 at 09:42:37PM +0100, Pavel Machek wrote:
> > > 
> > > mmap in tidspbridge is missing range-checks. For now, make this
> > > interface root-only, so that it does not cause security problems.
> > 
> > Please fix this properly and don't paper over the real problem here.
> 
> Well, if the driver is left uncompilable, I'm pretty sure it will
> bitrot.
> 
> I do have the hardware, but I'm pretty sure current mailine does not
> have enough support to boot Maemo, so it is non trivial for me to test
> changes here.
> 
> And yes, I'd like to get N900 to better shape, but there's more urgent
> work to do there. Such as "make sure N900 still boots once omap moves away
> from device files".
> 
> [It seems like check should be that
> 
> vma->vm_pgoff << PAGE_SHIFT >= pdata->phys_mempool_base and 
> vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT - pdata->phys_mempool_base) <= pdata->phys_mempool_size .
> 
> But... this is some kind of DSP coprocessor, and I am not sure if
> just exposing its address space to untrusted processes is good idea.
> 
> Heck, are you sure this is security problem in the first place? Yes,
> it is unchecked mmap. So what? If the device is 600 root.root, and if
> the DSP can take over main system, 
> 
>         if (!capable(CAP_SYS_RAWIO))
>                 return -EPERM;

Will that break userspace?  Who opens and mmaps this device?  If you
don't know if users do this, how can you say this patch isn't going to
break things just as much as not having the driver there at all?

thanks,

greg k-h

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2340 bytes --]

On Sunday 01 December 2013 04:45:01 Greg KH wrote:
> On Sat, Nov 30, 2013 at 11:58:23PM +0100, Pavel Machek wrote:
> > On Sat 2013-11-30 14:05:53, Greg KH wrote:
> > > On Sat, Nov 30, 2013 at 09:42:37PM +0100, Pavel Machek wrote:
> > > > mmap in tidspbridge is missing range-checks. For now,
> > > > make this interface root-only, so that it does not
> > > > cause security problems.
> > > 
> > > Please fix this properly and don't paper over the real
> > > problem here.
> > 
> > Well, if the driver is left uncompilable, I'm pretty sure it
> > will bitrot.
> > 
> > I do have the hardware, but I'm pretty sure current mailine
> > does not have enough support to boot Maemo, so it is non
> > trivial for me to test changes here.
> > 
> > And yes, I'd like to get N900 to better shape, but there's
> > more urgent work to do there. Such as "make sure N900 still
> > boots once omap moves away from device files".
> > 
> > [It seems like check should be that
> > 
> > vma->vm_pgoff << PAGE_SHIFT >= pdata->phys_mempool_base and
> > vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT -
> > pdata->phys_mempool_base) <= pdata->phys_mempool_size .
> > 
> > But... this is some kind of DSP coprocessor, and I am not
> > sure if just exposing its address space to untrusted
> > processes is good idea.
> > 
> > Heck, are you sure this is security problem in the first
> > place? Yes, it is unchecked mmap. So what? If the device is
> > 600 root.root, and if the DSP can take over main system,
> > 
> >         if (!capable(CAP_SYS_RAWIO))
> >         
> >                 return -EPERM;
> 
> Will that break userspace?  Who opens and mmaps this device? 
> If you don't know if users do this, how can you say this
> patch isn't going to break things just as much as not having
> the driver there at all?
> 
> thanks,
> 
> greg k-h

Maemo and harmattan userspace using gst-dsp (gstreamer plugin)
for using tidspbridge. It opening /dev/DspBridge and then it calling:

mmap(NULL, seg->size, PROT_READ | PROT_WRITE, MAP_SHARED | 0x2000 /* MAP_LOCKED */, handle, seg->base_pa);

Because it is gstreamer plugin, every application can use it (under non root too).

Can you check if above problematic kernel code is what this gst-dsp mmap calling?

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Pavel Machek]

Hi!

On Sat 2013-11-30 19:45:01, Greg KH wrote:
> On Sat, Nov 30, 2013 at 11:58:23PM +0100, Pavel Machek wrote:
> > On Sat 2013-11-30 14:05:53, Greg KH wrote:
> > > On Sat, Nov 30, 2013 at 09:42:37PM +0100, Pavel Machek wrote:
> > > > 
> > > > mmap in tidspbridge is missing range-checks. For now, make this
> > > > interface root-only, so that it does not cause security problems.
> > > 
> > > Please fix this properly and don't paper over the real problem here.
> > 
> > Well, if the driver is left uncompilable, I'm pretty sure it will
> > bitrot.
> > 
> > I do have the hardware, but I'm pretty sure current mailine does not
> > have enough support to boot Maemo, so it is non trivial for me to test
> > changes here.
> > 
> > And yes, I'd like to get N900 to better shape, but there's more urgent
> > work to do there. Such as "make sure N900 still boots once omap moves away
> > from device files".
> > 
> > [It seems like check should be that
> > 
> > vma->vm_pgoff << PAGE_SHIFT >= pdata->phys_mempool_base and 
> > vma->vm_end - vma->vm_start + (vma->vm_pgoff << PAGE_SHIFT - pdata->phys_mempool_base) <= pdata->phys_mempool_size .
> > 
> > But... this is some kind of DSP coprocessor, and I am not sure if
> > just exposing its address space to untrusted processes is good idea.
> > 
> > Heck, are you sure this is security problem in the first place? Yes,
> > it is unchecked mmap. So what? If the device is 600 root.root, and if
> > the DSP can take over main system, 
> > 
> >         if (!capable(CAP_SYS_RAWIO))
> >                 return -EPERM;
> 
> Will that break userspace?  Who opens and mmaps this device?  If you
> don't know if users do this, how can you say this patch isn't going to
> break things just as much as not having the driver there at all?

On maemo, /dev/DspBridge is mode 666. I tried looking up with fuser
who might use it, but that one does not seem to work on maemo.

So yes, this would "break" existing users... OTOH maemo does not work
on mainline kernels, and never did. (Maemo is not open source).

Anyway, tell me what you prefer. We seem to have chicken and egg
problem here... I can create the patch but not test it.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 2950 bytes --]

On Sunday 01 December 2013 12:26:10 Pavel Machek wrote:
> Hi!
> 
> On Sat 2013-11-30 19:45:01, Greg KH wrote:
> > On Sat, Nov 30, 2013 at 11:58:23PM +0100, Pavel Machek wrote:
> > > On Sat 2013-11-30 14:05:53, Greg KH wrote:
> > > > On Sat, Nov 30, 2013 at 09:42:37PM +0100, Pavel Machek 
wrote:
> > > > > mmap in tidspbridge is missing range-checks. For now,
> > > > > make this interface root-only, so that it does not
> > > > > cause security problems.
> > > > 
> > > > Please fix this properly and don't paper over the real
> > > > problem here.
> > > 
> > > Well, if the driver is left uncompilable, I'm pretty sure
> > > it will bitrot.
> > > 
> > > I do have the hardware, but I'm pretty sure current
> > > mailine does not have enough support to boot Maemo, so it
> > > is non trivial for me to test changes here.
> > > 
> > > And yes, I'd like to get N900 to better shape, but there's
> > > more urgent work to do there. Such as "make sure N900
> > > still boots once omap moves away from device files".
> > > 
> > > [It seems like check should be that
> > > 
> > > vma->vm_pgoff << PAGE_SHIFT >= pdata->phys_mempool_base
> > > and vma->vm_end - vma->vm_start + (vma->vm_pgoff <<
> > > PAGE_SHIFT - pdata->phys_mempool_base) <=
> > > pdata->phys_mempool_size .
> > > 
> > > But... this is some kind of DSP coprocessor, and I am not
> > > sure if just exposing its address space to untrusted
> > > processes is good idea.
> > > 
> > > Heck, are you sure this is security problem in the first
> > > place? Yes, it is unchecked mmap. So what? If the device
> > > is 600 root.root, and if the DSP can take over main
> > > system,
> > > 
> > >         if (!capable(CAP_SYS_RAWIO))
> > >         
> > >                 return -EPERM;
> > 
> > Will that break userspace?  Who opens and mmaps this device?
> >  If you don't know if users do this, how can you say this
> > patch isn't going to break things just as much as not
> > having the driver there at all?
> 
> On maemo, /dev/DspBridge is mode 666. I tried looking up with
> fuser who might use it, but that one does not seem to work on
> maemo.
> 

Hi! See my previous email. gst-dsp plugin using /dev/DspBridge, 
so any application which using gstreamer for viewing videos will 
use it. Try for example builtin media player and some h264 video 
with low resolution. Or directly gst-launch.

> So yes, this would "break" existing users... OTOH maemo does
> not work on mainline kernels, and never did. (Maemo is not
> open source).
> 

If you apply some patches to kernel and also userspace, you can 
run Maemo with (patched) upstream kernel. Just install CSSU devel 
and kernel patches from linux-n900 tree. Then you can test it.

> Anyway, tell me what you prefer. We seem to have chicken and
> egg problem here... I can create the patch but not test it.
> 
> 								Pavel

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1128 bytes --]

On Saturday 30 November 2013 23:58:23 Pavel Machek wrote:
> On Sat 2013-11-30 14:05:53, Greg KH wrote:
> > On Sat, Nov 30, 2013 at 09:42:37PM +0100, Pavel Machek wrote:
> > > mmap in tidspbridge is missing range-checks. For now, make
> > > this interface root-only, so that it does not cause
> > > security problems.
> > 
> > Please fix this properly and don't paper over the real
> > problem here.
> 
> Well, if the driver is left uncompilable, I'm pretty sure it
> will bitrot.
> 

If you want to compile tidspbridge driver, you need this patch:
https://gitorious.org/linux-n900/linux-n900/commit/b9adde42d5351467fa9d281190213bb652499577

Removing BROKEN is not enough.

> I do have the hardware, but I'm pretty sure current mailine
> does not have enough support to boot Maemo, so it is non
> trivial for me to test changes here.
> 

Use linux-n900 tree, there are patches also for tidspbridge
driver, so old maemo and new harmattan userspace can use it. But
HD videos not working yet (due to another problem, search for
OMAPFB: CMA allocation failures).

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Ивайло Димитров]

I can (and will) test whatever patches is needed. Also will try to get rid of uuid helpers and send a patch today.

On Sun Dec   1 11:41:39 2013 Pali Rohár <pali.rohar@gmail.com> wrote:
> On Saturday 30 November 2013 23:58:23 Pavel Machek wrote:
> > On Sat 2013-11-30 14:05:53, Greg KH wrote:
> > > On Sat, Nov 30, 2013 at 09:42:37PM +0100, Pavel Machek wrote:
> > > > mmap in tidspbridge is missing range-checks. For now, make
> > > > this interface root-only, so that it does not cause
> > > > security problems.
> > > 
> > > Please fix this properly and don't paper over the real
> > > problem here.
> > 
> > Well, if the driver is left uncompilable, I'm pretty sure it
> > will bitrot.
> > 
> 
> If you want to compile tidspbridge driver, you need this patch:
> https://gitorious.org/linux-n900/linux-n900/commit/b9adde42d5351467fa9d281190213bb652499577
> 
> Removing BROKEN is not enough.
> 
> > I do have the hardware, but I'm pretty sure current mailine
> > does not have enough support to boot Maemo, so it is non
> > trivial for me to test changes here.
> > 
> 
> Use linux-n900 tree, there are patches also for tidspbridge
> driver, so old maemo and new harmattan userspace can use it. But
> HD videos not working yet (due to another problem, search for
> OMAPFB: CMA allocation failures).
> 
> -- 
> Pali Rohár
> pali.rohar@gmail.com

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Pavel Machek]

Hi!

> I can (and will) test whatever patches is needed. Also will try to get rid of uuid helpers and send a patch today.
> 

So ... here's patch that you can start from, and that may even work...

Thanks,
									Pavel

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
index 1aa4a3f..a8e86cf 100644
--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
@@ -258,7 +258,17 @@ 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;
+	int status;
+	struct omap_dsp_platform_data *pdata =
+	    omap_dspbridge_dev->dev.platform_data;
+	unsigned long start = vma->vm_pgoff << PAGE_SHIFT;
+		
+	if (start < pdata->phys_mempool_base)
+		return -EINVAL;
+
+	if (vma->vm_end - vma->vm_start + (start - pdata->phys_mempool_base) 
+	    > pdata->phys_mempool_size)
+		return -EINVAL;
 
 	/* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
@@ -271,10 +281,10 @@ static int bridge_mmap(struct file *filp, struct vm_area_struct *vma)
 	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;
+	if (status)
+		return -EAGAIN;
 
-	return status;
+	return 0;
 }
 
 static const struct file_operations bridge_fops = {


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Dan Carpenter]

On Sun, Dec 01, 2013 at 01:10:06PM +0100, Pavel Machek wrote:
> diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
> index 1aa4a3f..a8e86cf 100644
> --- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
> +++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
> @@ -258,7 +258,17 @@ 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;
> +	int status;
> +	struct omap_dsp_platform_data *pdata =
> +	    omap_dspbridge_dev->dev.platform_data;
> +	unsigned long start = vma->vm_pgoff << PAGE_SHIFT;
> +		
> +	if (start < pdata->phys_mempool_base)
> +		return -EINVAL;
> +
> +	if (vma->vm_end - vma->vm_start + (start - pdata->phys_mempool_base) 
> +	    > pdata->phys_mempool_size)

This test is vulnerable to integer overflows if you pick a very high
value for start.  Consider using the vm_iomap_memory() helper function
instead of calling remap_pfn_range() directly.  Commit 7314e613d5ff
('Fix a few incorrectly checked [io_]remap_pfn_range() calls') has an
example of how the conversion works.

regards,
dan carpenter

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Ivajlo Dimitrov]

On 01.12.2013 14:27, Dan Carpenter wrote:
> On Sun, Dec 01, 2013 at 01:10:06PM +0100, Pavel Machek wrote:
>> diff --git a/drivers/staging/tidspbridge/rmgr/drv_interface.c b/drivers/staging/tidspbridge/rmgr/drv_interface.c
>> index 1aa4a3f..a8e86cf 100644
>> --- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
>> +++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
>> @@ -258,7 +258,17 @@ 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;
>> +	int status;
>> +	struct omap_dsp_platform_data *pdata =
>> +	    omap_dspbridge_dev->dev.platform_data;
>> +	unsigned long start = vma->vm_pgoff << PAGE_SHIFT;
>> +		
>> +	if (start < pdata->phys_mempool_base)
>> +		return -EINVAL;
>> +
>> +	if (vma->vm_end - vma->vm_start + (start - pdata->phys_mempool_base)
>> +	    > pdata->phys_mempool_size)
> This test is vulnerable to integer overflows if you pick a very high
> value for start.  Consider using the vm_iomap_memory() helper function
> instead of calling remap_pfn_range() directly.  Commit 7314e613d5ff
> ('Fix a few incorrectly checked [io_]remap_pfn_range() calls') has an
> example of how the conversion works.
>
> regards,
> dan carpenter
>
Dan,

If that one looks fine, I'll send a correctly formatted patch.

Pavel - feel free you to do it, I don't want to "steal" your bug :) .

Patch tested with Maemo5 on n900:

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

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Pavel Machek]

Hi!

> >>--- a/drivers/staging/tidspbridge/rmgr/drv_interface.c
> >>+++ b/drivers/staging/tidspbridge/rmgr/drv_interface.c
> >>@@ -258,7 +258,17 @@ 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;
> >>+	int status;
> >>+	struct omap_dsp_platform_data *pdata =
> >>+	    omap_dspbridge_dev->dev.platform_data;
> >>+	unsigned long start = vma->vm_pgoff << PAGE_SHIFT;
> >>+		
> >>+	if (start < pdata->phys_mempool_base)
> >>+		return -EINVAL;
> >>+
> >>+	if (vma->vm_end - vma->vm_start + (start - pdata->phys_mempool_base)
> >>+	    > pdata->phys_mempool_size)
> >This test is vulnerable to integer overflows if you pick a very high
> >value for start.  Consider using the vm_iomap_memory() helper function
> >instead of calling remap_pfn_range() directly.  Commit 7314e613d5ff
> >('Fix a few incorrectly checked [io_]remap_pfn_range() calls') has an
> >example of how the conversion works.
> >
> >regards,
> >dan carpenter
> >
> Dan,
> 
> If that one looks fine, I'll send a correctly formatted patch.

Looks good to me. Feel free to add

Signed-off-by: Pavel Machek <pavel@ucw.cz>
Reported-by: Nico Golde <nico@ngolde.de>
Reported-by: Fabian Yamaguchi <fabs@goesec.de>

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch] Staging: tidspbridge: make mmap root-only so it is not a security problem

[Dan Carpenter]

Looks good to me.

regards,
dan carpenter