Re: [PATCH RESEND] hwrng: core - don't pass stack allocated buffer to rng->read()
On 10/21/16 23:17, Richard W.M. Jones wrote: > On Fri, Oct 21, 2016 at 02:04:27PM -0700, Andy Lutomirski wrote: >> https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=6d4952d9d9d4dc2bb9c0255d95a09405a1e958f7 > > I have tested this one, and it also fixes the bug I was seeing. > > Thanks Laszlo as well for his fix, and sorry for not finding the > patch above first. No problem, it was fun :) -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND] hwrng: core - don't pass stack allocated buffer to rng->read()
On 10/21/16 23:04, Andy Lutomirski wrote: > On Fri, Oct 21, 2016 at 1:48 PM, Laszlo Ersek <ler...@redhat.com> wrote: >> The virtio-rng backend for hwrng passes the buffer that it receives for >> filling to sg_set_buf() directly, in: >> >> virtio_read() [drivers/char/hw_random/virtio-rng.c] >> register_buffer() [drivers/char/hw_random/virtio-rng.c] >> sg_init_one() [lib/scatterlist.c] >> sg_set_buf() [include/linux/scatterlist.h] >> >> In turn, the sg_set_buf() function, when built with CONFIG_DEBUG_SG, >> actively enforces (justifiedly) that the buffer used within the >> scatter-gather list live in physically contiguous memory: >> >> BUG_ON(!virt_addr_valid(buf)); >> >> The combination of the above two facts means that whatever calls >> virtio_read() -- via the hwrng.read() method -- has to allocate the >> recipient buffer in physically contiguous memory. > > Indeed. This bug should be fixed by: > > https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=6d4952d9d9d4dc2bb9c0255d95a09405a1e958f7 > Cool, thanks! (My commit message is better tho ;)) Cheers Laszlo -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND] hwrng: core - don't pass stack allocated buffer to rng->read()
1 5c 41 5d 5d c3 <0f> 0b
> 0f 0b 0f 0b 0f 0b 48 8b 15 05 ec 98 00 eb a3 0f 1f 00 55
> RIP [] sg_init_one+0x8c/0xa0
> RSP
> ---[ end trace 8120a17353b469c4 ]---
Prevent this by allocating a temporary buffer in add_early_randomness()
with kmalloc(). (The function add_early_randomness() should be called very
infrequently, therefore it makes sense to trade speed for storage; i.e.,
to allocate the buffer only temporarily, for every call separately.)
Cc: "Richard W.M. Jones" <rjo...@redhat.com>
Cc: <sta...@vger.kernel.org>
Cc: Amit Shah <amit.s...@redhat.com>
Cc: Andy Lutomirski <l...@kernel.org>
Cc: Herbert Xu <herb...@gondor.apana.org.au>
Cc: Kees Cook <keesc...@chromium.org>
Cc: Matt Mackall <m...@selenic.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1383451
Fixes: d9e797261933 ("hwrng: add randomness to system from rng sources")
See-also: 5e59d9a1aed2 ("virtio_console: Stop doing DMA on the stack")
Reported-by: "Richard W.M. Jones" <rjo...@redhat.com>
Tested-by: "Richard W.M. Jones" <rjo...@redhat.com>
Signed-off-by: Laszlo Ersek <ler...@redhat.com>
---
Notes:
- (GFP_NOWAIT | __GFP_NOWARN) could be overly cautious, but I'm better
safe than sorry.
- If / when responding, please keep me addressed personally; I'm not
subscribed to either linux-crypto or linux-kernel. Thanks.
drivers/char/hw_random/core.c | 28 ++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 482794526e8c..66831bd5331d 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -50,6 +50,7 @@
#define PFXRNG_MODULE_NAME ": "
#define RNG_MISCDEV_MINOR 183 /* official */
+#define EARLY_RANDOMNESS_SIZE 16
static struct hwrng *current_rng;
static struct task_struct *hwrng_fill;
@@ -84,14 +85,37 @@ static size_t rng_buffer_size(void)
static void add_early_randomness(struct hwrng *rng)
{
- unsigned char bytes[16];
+ unsigned char *bytes;
int bytes_read;
+ /*
+* This code can be reached with rng_mutex held, through the following
+* call chain:
+*
+* hwrng_attr_current_store()
+* set_current_rng()
+* hwrng_init()
+* add_early_randomness()
+*
+* (that is, when a different RNG is selected through the "rng_current"
+* sysfs attribute). For that reason, allocate memory without enabling
+* sleep.
+*
+* If the (immediate) allocation fails, we just pretend to have read
+* zero bytes from the RNG, as that is already valid behavior. Also,
+* feeding initial randomness from the device to the system entropy
+* pool is not important enough to tap into emergency memory pools.
+*/
+ bytes = kmalloc(EARLY_RANDOMNESS_SIZE, GFP_NOWAIT | __GFP_NOWARN);
+ if (!bytes)
+ return;
+
mutex_lock(_mutex);
- bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+ bytes_read = rng_get_data(rng, bytes, EARLY_RANDOMNESS_SIZE, 1);
mutex_unlock(_mutex);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
+ kfree(bytes);
}
static inline void cleanup_rng(struct kref *kref)
--
2.9.2
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwrng: core - don't pass stack allocated buffer to rng->read()
On 10/21/16 22:32, Laszlo Ersek wrote: > [...] Self-NAK, I'll resend in a minute. I added a tag like this: Cc: <sta...@vger.kernel.org> # For v3.15+ and git turned it into a garbage email address. I'll drop the "# For v3.15+" part in the repost. (I'll also add explicit quotes around Rich's name -- I had a suspicion that the dots wouldn't be correct without quoting. git-send-email armored them itself, but it also inserted gratuitous whitespace in front of the dots.) Sorry about the inconvenience. Thanks Laszlo -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] hwrng: core - don't pass stack allocated buffer to rng->read()
1 5c 41 5d 5d c3 <0f> 0b
> 0f 0b 0f 0b 0f 0b 48 8b 15 05 ec 98 00 eb a3 0f 1f 00 55
> RIP [] sg_init_one+0x8c/0xa0
> RSP
> ---[ end trace 8120a17353b469c4 ]---
Prevent this by allocating a temporary buffer in add_early_randomness()
with kmalloc(). (The function add_early_randomness() should be called very
infrequently, therefore it makes sense to trade speed for storage; i.e.,
to allocate the buffer only temporarily, for every call separately.)
Cc: <sta...@vger.kernel.org> # For v3.15+
Cc: Amit Shah <amit.s...@redhat.com>
Cc: Andy Lutomirski <l...@kernel.org>
Cc: Herbert Xu <herb...@gondor.apana.org.au>
Cc: Kees Cook <keesc...@chromium.org>
Cc: Matt Mackall <m...@selenic.com>
Cc: Richard W.M. Jones <rjo...@redhat.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1383451
Fixes: d9e797261933 ("hwrng: add randomness to system from rng sources")
See-also: 5e59d9a1aed2 ("virtio_console: Stop doing DMA on the stack")
Reported-by: Richard W.M. Jones <rjo...@redhat.com>
Tested-by: Richard W.M. Jones <rjo...@redhat.com>
Signed-off-by: Laszlo Ersek <ler...@redhat.com>
---
Notes:
- (GFP_NOWAIT | __GFP_NOWARN) could be overly cautious, but I'm better
safe than sorry.
- If / when responding, please keep me addressed personally; I'm not
subscribed to either linux-crypto or linux-kernel. Thanks.
drivers/char/hw_random/core.c | 28 ++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 482794526e8c..66831bd5331d 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -50,6 +50,7 @@
#define PFXRNG_MODULE_NAME ": "
#define RNG_MISCDEV_MINOR 183 /* official */
+#define EARLY_RANDOMNESS_SIZE 16
static struct hwrng *current_rng;
static struct task_struct *hwrng_fill;
@@ -84,14 +85,37 @@ static size_t rng_buffer_size(void)
static void add_early_randomness(struct hwrng *rng)
{
- unsigned char bytes[16];
+ unsigned char *bytes;
int bytes_read;
+ /*
+* This code can be reached with rng_mutex held, through the following
+* call chain:
+*
+* hwrng_attr_current_store()
+* set_current_rng()
+* hwrng_init()
+* add_early_randomness()
+*
+* (that is, when a different RNG is selected through the "rng_current"
+* sysfs attribute). For that reason, allocate memory without enabling
+* sleep.
+*
+* If the (immediate) allocation fails, we just pretend to have read
+* zero bytes from the RNG, as that is already valid behavior. Also,
+* feeding initial randomness from the device to the system entropy
+* pool is not important enough to tap into emergency memory pools.
+*/
+ bytes = kmalloc(EARLY_RANDOMNESS_SIZE, GFP_NOWAIT | __GFP_NOWARN);
+ if (!bytes)
+ return;
+
mutex_lock(_mutex);
- bytes_read = rng_get_data(rng, bytes, sizeof(bytes), 1);
+ bytes_read = rng_get_data(rng, bytes, EARLY_RANDOMNESS_SIZE, 1);
mutex_unlock(_mutex);
if (bytes_read > 0)
add_device_randomness(bytes, bytes_read);
+ kfree(bytes);
}
static inline void cleanup_rng(struct kref *kref)
--
2.9.2
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
