signal_pending() checks TIF_NOTIFY_SIGNAL and TIF_SIGPENDING, which signal
that the task should bail out of the syscall when possible.
This is a separate concept from need_resched(), which checks
TIF_NEED_RESCHED, signalling that the task should preempt.
In particular, with the current code, the signal_pending() bailout probably
won't work reliably.
Change this to look like other functions that read lots of data, such as
read_zero().
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Jann Horn <[email protected]>
---
drivers/char/random.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 1eb220a5f44f..7f0253455d4e 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -540,13 +540,13 @@ static ssize_t get_random_bytes_user(void __user *buf, size_t nbytes)
crng_make_state(chacha_state, (u8 *)&chacha_state[4], CHACHA_KEY_SIZE);
do {
- if (large_request && need_resched()) {
+ if (large_request) {
if (signal_pending(current)) {
if (!ret)
ret = -ERESTARTSYS;
break;
}
- schedule();
+ cond_resched();
}
chacha20_block(chacha_state, output);
base-commit: 0c3e7b36d92681bba4c73c198a35e5a806d6f3ff
--
2.35.1.1094.g7c7d902a7c-goog
Hi Jann,
On Tue, Apr 5, 2022 at 6:39 PM Jann Horn <[email protected]> wrote:
> signal_pending() checks TIF_NOTIFY_SIGNAL and TIF_SIGPENDING, which signal
> that the task should bail out of the syscall when possible.
> This is a separate concept from need_resched(), which checks
> TIF_NEED_RESCHED, signalling that the task should preempt.
>
> In particular, with the current code, the signal_pending() bailout probably
> won't work reliably.
>
> Change this to look like other functions that read lots of data, such as
> read_zero().
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Applied, thanks.
One funny aspect of the fact that signal_pending() hasn't worked right
since the genesis commit is that this has probably led to a lot of
userspace code that doesn't check the result from read() or
getrandom(), and that code has worked mostly fine.
I wonder if we should do something about that. Worth noting is that
we're no longer contending with /dev/random periodically blocking as
the "entropy runs out" nonsense. I can think of two possible changes,
which maybe aren't mutually exclusive:
1) Turn signal_pending() into fatal_signal_pending() throughout the file.
2) Rather than not checking signal_pending() for reads of length <=
256, we could not check for signal_pending() for the first 256 bytes
of any length read.
Both of these would be changing userspace behavior, so it should
probably be considered carefully. Any thoughts?
Jason
On 2022-04-05 20:07:27 [+0200], Jason A. Donenfeld wrote:
> One funny aspect of the fact that signal_pending() hasn't worked right
> since the genesis commit is that this has probably led to a lot of
> userspace code that doesn't check the result from read() or
> getrandom(), and that code has worked mostly fine.
:)
> I wonder if we should do something about that. Worth noting is that
> we're no longer contending with /dev/random periodically blocking as
> the "entropy runs out" nonsense. I can think of two possible changes,
> which maybe aren't mutually exclusive:
>
> 1) Turn signal_pending() into fatal_signal_pending() throughout the file.
> 2) Rather than not checking signal_pending() for reads of length <=
> 256, we could not check for signal_pending() for the first 256 bytes
> of any length read.
>
> Both of these would be changing userspace behavior, so it should
> probably be considered carefully. Any thoughts?
You are not doing any blocking as far as I can tell so there won't be
any wake up via TASK_INTERRUPTIBLE for you here.
You check for the signal_pending() every PAGE_SIZE so there will be at
least 4KiB of data, not sure where this 256 is coming from.
Since you always return the number of bytes, there won't be any visible
change for requests < PAGE_SIZE. And for requests > PAGE_SIZE your
getrandom() invocation may return less than asked for. This is _now_.
If you drop that signal_pending() check then the user will always get
the number of bytes he asked for. Given that this is *quick* as in
no blocking, then there should be no harm in dropping this signal check.
> Jason
Sebastian
Hi Sebastian,
On Fri, Jun 17, 2022 at 06:48:42PM +0200, Sebastian Siewior wrote:
> On 2022-04-05 20:07:27 [+0200], Jason A. Donenfeld wrote:
> > One funny aspect of the fact that signal_pending() hasn't worked right
> > since the genesis commit is that this has probably led to a lot of
> > userspace code that doesn't check the result from read() or
> > getrandom(), and that code has worked mostly fine.
>
> :)
>
> > I wonder if we should do something about that. Worth noting is that
> > we're no longer contending with /dev/random periodically blocking as
> > the "entropy runs out" nonsense. I can think of two possible changes,
> > which maybe aren't mutually exclusive:
> >
> > 1) Turn signal_pending() into fatal_signal_pending() throughout the file.
> > 2) Rather than not checking signal_pending() for reads of length <=
> > 256, we could not check for signal_pending() for the first 256 bytes
> > of any length read.
> >
> > Both of these would be changing userspace behavior, so it should
> > probably be considered carefully. Any thoughts?
>
> You are not doing any blocking as far as I can tell so there won't be
> any wake up via TASK_INTERRUPTIBLE for you here.
> You check for the signal_pending() every PAGE_SIZE so there will be at
> least 4KiB of data, not sure where this 256 is coming from.
> Since you always return the number of bytes, there won't be any visible
> change for requests < PAGE_SIZE. And for requests > PAGE_SIZE your
> getrandom() invocation may return less than asked for. This is _now_.
>
> If you drop that signal_pending() check then the user will always get
> the number of bytes he asked for. Given that this is *quick* as in
> no blocking, then there should be no harm in dropping this signal check.
You're a bit late to the thread :). It used to be 256. Now it's page
size. PAGE_SIZE is also what /dev/zero and others in mem.c use.
As for your suggestion to drop it entirely: that'd be nice, in that it'd
add a guarantee that currently doesn't exist. But it can lead to
somewhat large delays if somebody tries to read 2 gigabytes at a time
and hits Ctrl+C during it. That seems potentially bad?
Or that's not bad, which would be quite nice, as I would really love to
add that guarantee. So if you have an argument that not responding to
signals for that amount of time is fine, I'd be interested to hear it.
Jason
On 2022-06-18 00:47:12 [+0200], Jason A. Donenfeld wrote:
> Hi Sebastian,
Hi Jason,
> You're a bit late to the thread :). It used to be 256. Now it's page
> size. PAGE_SIZE is also what /dev/zero and others in mem.c use.
Just managed to get to that part of the inbox ;)
> As for your suggestion to drop it entirely: that'd be nice, in that it'd
> add a guarantee that currently doesn't exist. But it can lead to
> somewhat large delays if somebody tries to read 2 gigabytes at a time
> and hits Ctrl+C during it. That seems potentially bad?
So on my x86 box which runs a Debian kernel (based on v5.18.2):
| ~$ dd if=/dev/random of=/dev/null bs=2147483648 count=1
| 0+1 records in
| 0+1 records out
| 2147479552 bytes (2,1 GB, 2,0 GiB) copied, 5,97452 s, 359 MB/s
almost 6 secs. On a smaller box it might take 12s or more. Your
implementation change ensured that it does not block for unpredicted
amount of time. Previously it would block until the random pool is
filled with enough entropy and this could take an unforeseen amount of
time. That read now makes more or less constant progress since it
depends only on CPU time.
Based on that, I don't see a problem dropping that signal check
especially that requests larger than 4KiB are most likely exotic.
> Or that's not bad, which would be quite nice, as I would really love to
> add that guarantee. So if you have an argument that not responding to
> signals for that amount of time is fine, I'd be interested to hear it.
Just my two cents.
> Jason
Sebastian
Hi Sebastian,
On Mon, Jun 20, 2022 at 09:43:56AM +0200, Sebastian Siewior wrote:
> > As for your suggestion to drop it entirely: that'd be nice, in that it'd
> > add a guarantee that currently doesn't exist. But it can lead to
> > somewhat large delays if somebody tries to read 2 gigabytes at a time
> > and hits Ctrl+C during it. That seems potentially bad?
>
> So on my x86 box which runs a Debian kernel (based on v5.18.2):
>
> | ~$ dd if=/dev/random of=/dev/null bs=2147483648 count=1
> | 0+1 records in
> | 0+1 records out
> | 2147479552 bytes (2,1 GB, 2,0 GiB) copied, 5,97452 s, 359 MB/s
>
> almost 6 secs. On a smaller box it might take 12s or more. Your
> implementation change ensured that it does not block for unpredicted
> amount of time. Previously it would block until the random pool is
> filled with enough entropy and this could take an unforeseen amount of
> time. That read now makes more or less constant progress since it
> depends only on CPU time.
> Based on that, I don't see a problem dropping that signal check
> especially that requests larger than 4KiB are most likely exotic.
I don't have a huge objection to that, but I also don't really know
what's normal and expected behavior here. What do you make of the usage
of should_stop_iteration() in drivers/char/mem.c, for example? Would you
also remove that? If you send a patch to change this in random.c,
perhaps you should also change it in mem.c and elsewhere, so that some
broader consensus forms (or doesn't form) on what the expected behavior
is.
Jason
On Mon, Jun 20, 2022 at 2:44 AM Sebastian Siewior <[email protected]> wrote:
>
> Based on that, I don't see a problem dropping that signal check
> especially that requests larger than 4KiB are most likely exotic.
Why would we do that?
Anybody who doesn't handle -EINTR is a clown, not a security issue.
Your "6s isn't that bad" is ridiculous, since
(a) 6 seconds is forever
(b) there are issues like "oops, we're out of memory, you got a
signal because root is trying to kill your annoying stupid program
using top"
and the fact is, anybody who asks for more than a few kilo-*bits* from
the kernel is already doing something questionable to begin with, and
there is no reason to bend over backwards and try to make such a crazy
use suddenly act differently from ALL OTHER character devices.
Handling signals is the *default* behavior. It is only regular files
where that doesn't happen. This is not a regular file, and the "it's
about security" is not an argument.
As mentioned, expecting an uninterruptible read is not "security". It's garbage.
Linus
On Mon, Jun 20, 2022 at 02:00:18PM -0500, Linus Torvalds wrote:
> Handling signals is the *default* behavior. It is only regular files
> where that doesn't happen. This is not a regular file, and the "it's
> about security" is not an argument.
>
> As mentioned, expecting an uninterruptible read is not "security". It's garbage.
Indeed, practically speaking you are right. A few months back when I was
fixing this to respect signals in the first place (because it used to be
wrongly conditioned on need_resched()), I did some global code searches
to see if I'd break anything, and basically it seemed like anything that
used huge buffers handled EINTR, and only little buffers weren't filled
iteratively. So PAGE_SIZE would seem to widely handle the real code out
there. If this is to change as an API ease of use argument or
something, I'd expect the behavior to change across devices (/dev/zero
and such I mentioned earlier in mem.c), not just this one -- no need to
be a special snowflake -- which means convincing people to change
something pretty well established.
Jason
On 2022-06-20 14:00:18 [-0500], Linus Torvalds wrote:
>
> As mentioned, expecting an uninterruptible read is not "security". It's garbage.
right.
> Linus
Sebastian