805154 - daily builds do not have the ability to gather memory reports

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[[e-mail] [:onecyrenus]]

Daily builds currently don't have the ability to gather memory reports. 

The basics behind this: 

/system/bin/killer application needs to be installed from gonk-misc in order to trigger a memory dump of b2g. 

The current reason why it is not part of the nightly builds is because there is a security hole which requires killer to be launched with root access. 

This bug is to fix the killer application so it doesn't require root in order to trigger a memory dump, or gc.

Comment 1

[Justin Lebar (not reading bugmail)]

> This bug is to fix the killer application so it doesn't require root in order to trigger 
> a memory dump, or gc.

ISTM that if we made killer "safe" (in that it could only send certain signals to certain processes) that we could setuid root and be done.  That is, I don't think making killer not require root is a necessary condition here.

Comment 2

[Dave Hylands [:dhylands]]

I confirmed that using a setuid root version of killer works when run from and adb shell that was owned by the user shell.

Comment 3

[Dave Hylands [:dhylands]]

Attachment: WIP (patch to b2g/gonk-misc): One possible way

This is one possible way to achieve the goal. This particular solution has the negative side-effect that killer would need to be marked as setuid root (which would need to happen in init.rc) in order to work on a production phone (where adb shell runs as user shell rather than user root)

Comment 4

[Justin Lebar (not reading bugmail)]

Comment on attachment 674867 [details] [diff] [review]
WIP (patch to b2g/gonk-misc): One possible way

Since killer will be setuid(0), we need to cast a paranoid eye on it.

So to be paranoid: A user can largely control which PID a new process gets.  If I can cause the B2G or plugin-container process to die at just the right time, I can cause this program to send a signal to an arbitrary new process.

Is there something we can do about this?

Comment 5

[Dave Hylands [:dhylands]]

So, you'd have to launch a process which was also setuid(0) and it would have to do something bad upon receiving one of the signals that we send. Because you can use regular kill on the non-setuid(0) programs.

I don't see anyway to close the window between checking the pid and issuing the kill. In kernel land you could disable preemption, but from user-space you can't do that.

The "right" way to deal with this is to probably abandon the killer approach and use a named-pipe. Then there won't be the same security holes.

Comment 6

[Justin Lebar (not reading bugmail)]

> So, you'd have to launch a process which was also setuid(0) and it would have to do 
> something bad upon receiving one of the signals that we send. Because you can use regular 
> kill on the non-setuid(0) programs.

Right.  I think the default action for a realtime signal is to crash.

> The "right" way to deal with this is to probably abandon the killer approach and use a 
> named-pipe. Then there won't be the same security holes.

:-/  That's probably right.

The named pipes thing wouldn't be hard, but we need this now, so let's do this as you've written and we can worry about named pipes later.

Comment 7

[Justin Lebar (not reading bugmail)]

Comment on attachment 674867 [details] [diff] [review]
WIP (patch to b2g/gonk-misc): One possible way

r=me, but please update the usage and comment at the top of the file to indicate that this is a severely restricted clone of kill.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Getting memory reports is a blocker. But of course we should make sure that the mechanism to do so is safe.

Comment 9

[Dave Hylands [:dhylands]]

Making killer part of the user builds has landed.
See:
https://github.com/mozilla-b2g/gonk-misc/pull/47
https://github.com/mozilla-b2g/platform_build/pull/9

The patch attached to this bug (with the comments and usage updated) is here:
https://github.com/mozilla-b2g/gonk-misc/pull/48

By adding setuid root (which needs to be done as part of the kernel/initial ramdisk) needs to be done elsewhere. If this variant of killer is made setuid root, it will allow memory reports to be gathered on a production phone where adb shell is owned by the shell user (currently adb shell is owned by the root user)

Comment 10

[Dave Hylands [:dhylands]]

Commit
https://github.com/mozilla-b2g/platform_build/commit/dfc13fee7c539005f869e1335e732e700aa42768
makes killer part of the build

So I'm going to close this bug out.