Bug 80981 - read of /dev/urandom causes kernel panic
Summary: read of /dev/urandom causes kernel panic
Status: CLOSED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: Other (show other bugs)
Hardware: x86-64 Linux
: P1 normal
Assignee: drivers_other
URL:
Keywords:
: 80991 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-07-23 10:03 UTC by aCaB
Modified: 2015-04-04 19:52 UTC (History)
5 users (show)

See Also:
Kernel Version: 3.15.4
Subsystem:
Regression: No
Bisected commit-id:


Attachments
trace photo (397.13 KB, image/jpeg)
2014-07-23 10:03 UTC, aCaB
Details
better picture (446.31 KB, image/jpeg)
2014-07-23 10:14 UTC, aCaB
Details

Description aCaB 2014-07-23 10:03:10 UTC
Created attachment 144001 [details]
trace photo

Third time in two days my kernel panics while issuing the following command as user:
dd if=/dev/urandom of=myfile bs=100M count=1

Everytime the trace looks more or less like the one attached.
Sorry if this is not cpuidle related, it's just a guess on my side based on the trace.
Comment 1 aCaB 2014-07-23 10:14:46 UTC
Created attachment 144011 [details]
better picture

Apparently this is very reproducible.
In fact it's just crashed again...
Comment 2 Andrey Utkin 2014-07-23 11:31:36 UTC
Could you please retry with kernel build from linux-next repo? There are recent changes regarding /dev/*random. git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
Comment 3 aCaB 2014-07-23 12:45:13 UTC
So i've done a quick test with 5eb00b037d9bb650b18b8f331bb9fb7a66559b5f (quickly rebuilt with make oldconfig and all the defaults accepted) and it does not panic.

However it also looks terribly broken in that /dev/urandom returns EOF after exactly 33554431 bytes (tried 10 times and happened every single time):
$ dd if=/dev/urandom of=myfile bs=100M count=1
0+1 records in
0+1 records out
33554431 bytes (34 MB) copied, 2.0128 s, 16.7 MB/s

On a side note kenrel 3.14.0 does not panic when dd reads from urandom.
Comment 4 Alexey Dobriyan 2014-07-23 13:16:33 UTC
33MB EOF reproduced with 15ba2236f3556fc01b9ca91394465152b5ea74b6
("Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net")
Comment 5 Andrey Utkin 2014-07-23 13:41:26 UTC
I believe the reason is commit 79a8468747c5f95ed3d5ce8376a3e82e0c5857fc , the chunk

@@ -1376,6 +1386,7 @@ urandom_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
                            "with %d bits of entropy available\n",
                            current->comm, nonblocking_pool.entropy_total);
 
+       nbytes = min_t(size_t, nbytes, INT_MAX >> (ENTROPY_SHIFT + 3));
        ret = extract_entropy_user(&nonblocking_pool, buf, nbytes);
 
        trace_urandom_read(8 * nbytes, ENTROPY_BITS(&nonblocking_pool),



which is likely what is described in the commit message as "additional paranoia check to prevent overly large count values to be passed into urandom_read()"
Comment 6 Andrey Utkin 2014-07-23 13:56:55 UTC
Emailed into LKML, Theodore & Hannes Frederic, see with subject "Reading large amounts from /dev/urandom broken".
Comment 7 Theodore Tso 2014-07-23 15:11:06 UTC
The bug should be fixed if you cherry-pick commit 79a8468747c5f95ed3d5ce8376a3e82e0c5857fc into 3.15.4+.   Please let me know if it doesn't.

Why on *earth* are you using /dev/urandom in this way?   The nbytes restriction is to prevent an accounting failure since we are now tracking entropy in fractional bits, so when we convert bytes requested into fractional bits, we overflow if someone tries to request more than 32MB.  Given that no sane use of /dev/urandom needs more than 256 bytes, this was considered acceptable.
Comment 8 aCaB 2014-07-23 18:01:58 UTC
(In reply to Theodore Tso from comment #7)
> The bug should be fixed if you cherry-pick commit
> 79a8468747c5f95ed3d5ce8376a3e82e0c5857fc into 3.15.4+.   Please let me know
> if it doesn't.

I confirm that appying 79a8468747c5f95ed3d5ce8376a3e82e0c5857fc onto v3.15.4 indeed fixes the ooops.
Thanks for the quick reply.


> Why on *earth* are you using /dev/urandom in this way?

Because I love doing stupid things. Because I can do stupid things.
And despite being stupid neither should be a valid reason to get a kernel panic, which is why I've opened this bug.

Applying 79a8468747c5f95ed3d5ce8376a3e82e0c5857fc forbids me from doing stupid things, which is how it fixes this bug (apologies for calling it a broken behaviour, I see it's intended).


> The nbytes
> restriction is to prevent an accounting failure since we are now tracking
> entropy in fractional bits, so when we convert bytes requested into
> fractional bits, we overflow if someone tries to request more than 32MB. 

Thanks for the explanation.


> Given that no sane use of /dev/urandom needs more than 256 bytes, this was
> considered acceptable.

It certainly is, if it's properly enforced.

Thanks again!
Comment 9 Theodore Tso 2014-07-23 19:47:00 UTC
Well, read(2) is always allowed to return a short read.  So the change which is currently queued for 3.16 doesn't actually break anything.  The fact that dd treats a short read as an EOF is dd's problem.  Any C program that reads from /dev/urandom must always check for short reads, since if a signal interupts the read, you can get the short read.   The only difference is that now, if you try to give a length > 32MB, you are guaranteed to get a short read.
Comment 10 Alexey Dobriyan 2014-07-23 20:05:14 UTC
If dd doesn't check for short reads... :-)
Comment 11 aCaB 2014-07-23 20:07:48 UTC
Short reads are absolutely fine, it was me incorrectly assunming an EOF rather than dd failing to handle them properly.
In fact I've just found out that dd requires an extra "iflag=fullblock" option in order to do so.

Thanks again.
Comment 12 Alexey Dobriyan 2014-07-23 20:10:32 UTC
"Don't break userspace" doctrine says that short reads aren't fine.
Comment 13 Theodore Tso 2014-07-23 20:34:01 UTC
If you do something like dd if=/dev/urandom of=/dev/sdc bs=64M, it doesn't stop after 32MB.   The fact that we are now returning short reads means that certain very implementation behaviour changes will be *different*, but *different* is not the same as *breakage*.
Comment 14 Toralf Förster 2014-07-23 20:45:27 UTC
(In reply to Theodore Tso from comment #9)
> Well, read(2) is always allowed to return a short read.  So the change which
> is currently queued for 3.16 doesn't actually break anything.

I'm able to crash my desktop system (32bit x86 Gentoo stable with vanilla kernel 3.1.5.6) with this command:

$> dd if=/dev/urandom of=/dev/zero bs=67108707

as a local, unprivileged user.

Does the mentioned change prevent this crash ?
Comment 15 Alexey Dobriyan 2014-07-23 20:48:55 UTC
Toralf, make sure you pulled 79a8468747c5f95ed3d5ce8376a3e82e0c5857fc
"random: check for increase of entropy_count because of signed conversion"

it fixes crash
Comment 16 Theodore Tso 2014-07-23 21:44:40 UTC
There are two separate things being discussed in this bugzilla.  The first is the kernel BUG, which is fixed by 79a8468747c5, which has been accepted by Linus and cc'ed to stable@kernel.org, so it should eventually show up in a 3.15.x kernel.

The other discussion is a complaint that commit 79a8468747c5 causes reads larger than 32MB results in a only 32MB to be returned by the read(2) system call.  That is, it results in a short read.  POSIX always allows for a short read(2), and any program MUST check for short reads.

The problem with dd is that POSIX requires the count=X parameter, to be based on reads, not on bytes.  This can be changed with iflag=fullblock.

There is no legitimate use of /dev/urandom and dd, so I'm not particularly worried about small implementation-level behavioural changes, especially when all of this is POSIX complaint, and any sane program (a) wouldn't be reading more than 512 bytes (and usually, only 16 or 32 bytes is plenty), and (b) any competent program will check the return value of read(2) and deal with short reads.  If you want to erase a disk, using nwipe is much faster.  If you want to securely overwrite a file, the right tool to use is wipe.
Comment 17 Toralf Förster 2014-07-24 15:53:40 UTC
(In reply to Alexey Dobriyan from comment #15)
> Toralf, make sure you pulled 79a8468747c5f95ed3d5ce8376a3e82e0c5857fc
> "random: check for increase of entropy_count because of signed conversion"
> 
> it fixes crash
thx - applied on top of 3.15.6 - works fine
Comment 18 tester6382 2014-07-25 12:38:57 UTC
*** Bug 80991 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.