All of lore.kernel.org
 help / color / mirror / Atom feed
From: Martijn Coenen <maco@android.com>
To: gregkh@linuxfoundation.org, john.stultz@linaro.org,
	tkjos@google.com, arve@android.com, amit.pundir@linaro.org
Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
	maco@google.com, malchev@google.com, ccross@android.com,
	Martijn Coenen <maco@android.com>
Subject: [PATCH 08/13] ANDROID: binder: don't check prio permissions on restore.
Date: Fri, 25 Aug 2017 11:33:30 +0200	[thread overview]
Message-ID: <20170825093335.100892-9-maco@android.com> (raw)
In-Reply-To: <20170825093335.100892-1-maco@android.com>

Because we have disabled RT priority inheritance for
the regular binder domain, the following can happen:

1) thread A (prio 98) calls into thread B
2) because RT prio inheritance is disabled, thread B
   runs at the lowest nice (prio 100) instead
3) thread B calls back into A; A will run at prio 100
   for the duration of the transaction
4) When thread A is done with the call from B, we will
   try to restore the prio back to 98. But, we fail
   because the process doesn't hold CAP_SYS_NICE,
   neither is RLIMIT_RT_PRIO set.

While the proper fix going forward will be to
correctly apply CAP_SYS_NICE or RLIMIT_RT_PRIO,
for now it seems reasonable to not check permissions
on the restore path.

Signed-off-by: Martijn Coenen <maco@android.com>
---
 drivers/android/binder.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 8eee4716e777..d6648b5f5988 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -1108,8 +1108,9 @@ static int to_kernel_prio(int policy, int user_priority)
 		return MAX_USER_RT_PRIO - 1 - user_priority;
 }
 
-static void binder_set_priority(struct task_struct *task,
-				struct binder_priority desired)
+static void binder_do_set_priority(struct task_struct *task,
+				   struct binder_priority desired,
+				   bool verify)
 {
 	int priority; /* user-space prio value */
 	bool has_cap_nice;
@@ -1122,7 +1123,7 @@ static void binder_set_priority(struct task_struct *task,
 
 	priority = to_userspace_prio(policy, desired.prio);
 
-	if (is_rt_policy(policy) && !has_cap_nice) {
+	if (verify && is_rt_policy(policy) && !has_cap_nice) {
 		long max_rtprio = task_rlimit(task, RLIMIT_RTPRIO);
 
 		if (max_rtprio == 0) {
@@ -1133,7 +1134,7 @@ static void binder_set_priority(struct task_struct *task,
 		}
 	}
 
-	if (is_fair_policy(policy) && !has_cap_nice) {
+	if (verify && is_fair_policy(policy) && !has_cap_nice) {
 		long min_nice = rlimit_to_nice(task_rlimit(task, RLIMIT_NICE));
 
 		if (min_nice > MAX_NICE) {
@@ -1166,6 +1167,18 @@ static void binder_set_priority(struct task_struct *task,
 		set_user_nice(task, priority);
 }
 
+static void binder_set_priority(struct task_struct *task,
+				struct binder_priority desired)
+{
+	binder_do_set_priority(task, desired, /* verify = */ true);
+}
+
+static void binder_restore_priority(struct task_struct *task,
+				    struct binder_priority desired)
+{
+	binder_do_set_priority(task, desired, /* verify = */ false);
+}
+
 static void binder_transaction_priority(struct task_struct *task,
 					struct binder_transaction *t,
 					struct binder_priority node_prio,
@@ -3202,7 +3215,7 @@ static void binder_transaction(struct binder_proc *proc,
 		binder_enqueue_work_ilocked(&t->work, &target_thread->todo);
 		binder_inner_proc_unlock(target_proc);
 		wake_up_interruptible_sync(&target_thread->wait);
-		binder_set_priority(current, in_reply_to->saved_priority);
+		binder_restore_priority(current, in_reply_to->saved_priority);
 		binder_free_transaction(in_reply_to);
 	} else if (!(t->flags & TF_ONE_WAY)) {
 		BUG_ON(t->buffer->async_transaction != 0);
@@ -3291,7 +3304,7 @@ static void binder_transaction(struct binder_proc *proc,
 
 	BUG_ON(thread->return_error.cmd != BR_OK);
 	if (in_reply_to) {
-		binder_set_priority(current, in_reply_to->saved_priority);
+		binder_restore_priority(current, in_reply_to->saved_priority);
 		thread->return_error.cmd = BR_TRANSACTION_COMPLETE;
 		binder_enqueue_work(thread->proc,
 				    &thread->return_error.work,
@@ -3891,7 +3904,7 @@ static int binder_thread_read(struct binder_proc *proc,
 			wait_event_interruptible(binder_user_error_wait,
 						 binder_stop_on_user_error < 2);
 		}
-		binder_set_priority(current, proc->default_priority);
+		binder_restore_priority(current, proc->default_priority);
 	}
 
 	if (non_block) {
-- 
2.14.1.480.gb18f417b89-goog

  parent reply	other threads:[~2017-08-25  9:35 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-25  9:33 [PATCH 00/13] ANDROID: binder: RT priority inheritance and small fixes Martijn Coenen
2017-08-25  9:33 ` [PATCH 01/13] ANDROID: binder: remove proc waitqueue Martijn Coenen
2017-08-25  9:33 ` [PATCH 02/13] ANDROID: binder: push new transactions to waiting threads Martijn Coenen
2017-08-25  9:33 ` [PATCH 03/13] ANDROID: binder: add support for RT prio inheritance Martijn Coenen
2017-08-25 15:08   ` Thomas Gleixner
2017-08-25 18:47     ` Martijn Coenen
2017-08-25  9:33 ` [PATCH 04/13] ANDROID: binder: add min sched_policy to node Martijn Coenen
2017-08-25  9:33 ` [PATCH 05/13] ANDROID: binder: improve priority inheritance Martijn Coenen
2017-08-25  9:33 ` [PATCH 06/13] ANDROID: binder: add RT inheritance flag to node Martijn Coenen
2017-08-25  9:33 ` [PATCH 07/13] Add BINDER_GET_NODE_DEBUG_INFO ioctl Martijn Coenen
2017-08-25  9:33 ` Martijn Coenen [this message]
2017-08-25  9:33 ` [PATCH 09/13] ANDROID: binder: Don't BUG_ON(!spin_is_locked()) Martijn Coenen
2017-08-25  9:33 ` [PATCH 10/13] ANDROID: binder: call poll_wait() unconditionally Martijn Coenen
2017-08-25  9:33 ` [PATCH 11/13] ANDROID: binder: don't enqueue death notifications to thread todo Martijn Coenen
2017-08-25  9:33 ` [PATCH 12/13] ANDROID: binder: don't queue async transactions to thread Martijn Coenen
2017-08-25  9:33 ` [PATCH 13/13] ANDROID: binder: Add tracing for binder priority inheritance Martijn Coenen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170825093335.100892-9-maco@android.com \
    --to=maco@android.com \
    --cc=amit.pundir@linaro.org \
    --cc=arve@android.com \
    --cc=ccross@android.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=john.stultz@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maco@google.com \
    --cc=malchev@google.com \
    --cc=tkjos@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.