diff mbox

timerfd: use-after-free in timerfd_remove_cancel

Message ID alpine.DEB.2.20.1701311231390.3457@nanos (mailing list archive)
State New, archived
Headers show

Commit Message

Thomas Gleixner Jan. 31, 2017, 11:33 a.m. UTC 
On Mon, 30 Jan 2017, Dmitry Vyukov wrote:
> 
> Seems that ctx->might_cancel is racy.

Yes, it is. Fix below.

8<-------------------

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Thomas Gleixner Jan. 31, 2017, 11:45 a.m. UTC | #1 
On Tue, 31 Jan 2017, Thomas Gleixner wrote:

> On Mon, 30 Jan 2017, Dmitry Vyukov wrote:
> > 
> > Seems that ctx->might_cancel is racy.
> 
> Yes, it is. Fix below.

And the fix is racy as well. Darn, we really need to lock the context to
avoid that mess.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Vyukov Jan. 31, 2017, 12:09 p.m. UTC | #2 
On Tue, Jan 31, 2017 at 12:45 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 31 Jan 2017, Thomas Gleixner wrote:
>
>> On Mon, 30 Jan 2017, Dmitry Vyukov wrote:
>> >
>> > Seems that ctx->might_cancel is racy.
>>
>> Yes, it is. Fix below.
>
> And the fix is racy as well. Darn, we really need to lock the context to
> avoid that mess.

Yes. I think we need to lock most of timerfd_settime. Otherwise we can
end up with a timer that needs to be in the cancel list, but it is
actually not; or vice versa.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -40,9 +40,12 @@  struct timerfd_ctx {
 	short unsigned settime_flags;	/* to show in fdinfo */
 	struct rcu_head rcu;
 	struct list_head clist;
-	bool might_cancel;
+	unsigned long flags;
 };
 
+/* Bit positions for ctx->flags */
+#define MIGHT_CANCEL	0
+
 static LIST_HEAD(cancel_list);
 static DEFINE_SPINLOCK(cancel_lock);
 
@@ -99,7 +102,7 @@  void timerfd_clock_was_set(void)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(ctx, &cancel_list, clist) {
-		if (!ctx->might_cancel)
+		if (!test_bit(MIGHT_CANCEL, &ctx->flags))
 			continue;
 		spin_lock_irqsave(&ctx->wqh.lock, flags);
 		if (ctx->moffs != moffs) {
@@ -114,8 +117,7 @@  void timerfd_clock_was_set(void)
 
 static void timerfd_remove_cancel(struct timerfd_ctx *ctx)
 {
-	if (ctx->might_cancel) {
-		ctx->might_cancel = false;
+	if (test_and_clear_bit(MIGHT_CANCEL, &ctx->flags)) {
 		spin_lock(&cancel_lock);
 		list_del_rcu(&ctx->clist);
 		spin_unlock(&cancel_lock);
@@ -124,7 +126,7 @@  static void timerfd_remove_cancel(struct
 
 static bool timerfd_canceled(struct timerfd_ctx *ctx)
 {
-	if (!ctx->might_cancel || ctx->moffs != KTIME_MAX)
+	if (!test_bit(MIGHT_CANCEL, &ctx->flags) || ctx->moffs != KTIME_MAX)
 		return false;
 	ctx->moffs = ktime_mono_to_real(0);
 	return true;
@@ -135,13 +137,12 @@  static void timerfd_setup_cancel(struct
 	if ((ctx->clockid == CLOCK_REALTIME ||
 	     ctx->clockid == CLOCK_REALTIME_ALARM) &&
 	    (flags & TFD_TIMER_ABSTIME) && (flags & TFD_TIMER_CANCEL_ON_SET)) {
-		if (!ctx->might_cancel) {
-			ctx->might_cancel = true;
+		if (test_and_set_bit(MIGHT_CANCEL, &ctx->flags)) {
 			spin_lock(&cancel_lock);
 			list_add_rcu(&ctx->clist, &cancel_list);
 			spin_unlock(&cancel_lock);
 		}
-	} else if (ctx->might_cancel) {
+	} else {
 		timerfd_remove_cancel(ctx);
 	}
 }