linux-kernel - Re: [PATCH] random: Fix the issue of '_might_sleep' function running in an atomic contex

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zfo3QUFEy4wHkLEB@zx2c4.com>
Date: Wed, 20 Mar 2024 02:09:21 +0100
From: "Jason A. Donenfeld" <Jason@...c4.com>
To: Guoyong Wang <guoyong.wang@...iatek.com>
Cc: Theodore Ts'o <tytso@....edu>, Tejun Heo <tj@...nel.org>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, wsd_upstream@...iatek.com
Subject: Re: [PATCH] random: Fix the issue of '_might_sleep' function running
 in an atomic contex

On Tue, Mar 19, 2024 at 05:30:55PM +0800, Guoyong Wang wrote:
> On Mon, 18 Mar 2024 21:00:42 +0100, Jason A. Donenfeld wrote:
> > I'm wondering, though, rather than introducing a second function, maybe
> > execute_in_process_context() should just gain a `&& !in_atomic()`.
> > That'd make things a bit simpler.  
> 
> > However, I'm pretty sure in_atomic() isn't actually a reliable way of
> > determining that, depending on config. So maybe this should just call
> > the worker always (if system_wq isn't null).
> 
> > Alternatively, any chance the call to add_input_randomness() could be
> > moved outside the spinlock, or does this not look possible?
> 
> Hi Jason,
> 
> Thanks for your suggestions. 
> 
> I am inclined to accept your second suggestion. My reluctance to accept 
> the first is due to the concern that "&& !in_atomic()" could potentially 
> alter the original meaning of the 'execute_in_process_context' interface. 
> Regarding the third suggestion, modifying the logic associated with 'input' 
> is not recommended.

Doesn't something like the below seem simplest? Just move the call out
of the spinlock and we're done.

diff --git a/drivers/input/input-core-private.h b/drivers/input/input-core-private.h
index 116834cf8868..717f239e28d0 100644
--- a/drivers/input/input-core-private.h
+++ b/drivers/input/input-core-private.h
@@ -10,7 +10,7 @@
 struct input_dev;

 void input_mt_release_slots(struct input_dev *dev);
-void input_handle_event(struct input_dev *dev,
+bool input_handle_event(struct input_dev *dev,
 			unsigned int type, unsigned int code, int value);

 #endif /* _INPUT_CORE_PRIVATE_H */
diff --git a/drivers/input/input.c b/drivers/input/input.c
index f71ea4fb173f..2faf46218c66 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -391,20 +391,22 @@ static void input_event_dispose(struct input_dev *dev, int disposition,
 	}
 }

-void input_handle_event(struct input_dev *dev,
+bool input_handle_event(struct input_dev *dev,
 			unsigned int type, unsigned int code, int value)
 {
 	int disposition;
+	bool should_contribute_to_rng = false;

 	lockdep_assert_held(&dev->event_lock);

 	disposition = input_get_disposition(dev, type, code, &value);
 	if (disposition != INPUT_IGNORE_EVENT) {
 		if (type != EV_SYN)
-			add_input_randomness(type, code, value);
+			should_contribute_to_rng = true;

 		input_event_dispose(dev, disposition, type, code, value);
 	}
+	return should_contribute_to_rng;
 }

 /**
@@ -428,12 +430,15 @@ void input_event(struct input_dev *dev,
 		 unsigned int type, unsigned int code, int value)
 {
 	unsigned long flags;
+	bool should_contribute_to_rng;

 	if (is_event_supported(type, dev->evbit, EV_MAX)) {

 		spin_lock_irqsave(&dev->event_lock, flags);
-		input_handle_event(dev, type, code, value);
+		should_contribute_to_rng = input_handle_event(dev, type, code, value);
 		spin_unlock_irqrestore(&dev->event_lock, flags);
+		if (should_contribute_to_rng)
+			add_input_randomness(type, code, value);
 	}
 }
 EXPORT_SYMBOL(input_event);