| Message ID | e958f214e8885968be8045ffde813ac339b81178.1481575835.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
Andy Lutomirski <luto@kernel.org> wrote: > +static const char zero_pad[16] = {0}; Isn't there a global page of zeros or something that we can share? Also, you shouldn't explicitly initialise it so that it stays in .bss. > - sg_set_buf(&sg_out[1], pad, sizeof pad); > + sg_set_buf(&sg_out[1], zero_pad, sizeof zero_pad); Can you put brackets on the sizeof? Thanks, David -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 12, 2016 at 2:28 PM, David Howells <dhowells@redhat.com> wrote: > Andy Lutomirski <luto@kernel.org> wrote: > >> +static const char zero_pad[16] = {0}; > > Isn't there a global page of zeros or something that we can share? Also, you > shouldn't explicitly initialise it so that it stays in .bss. This is a double-edged sword. It seems that omitting the initialization causes it to go in .bss, which isn't read only. I have no idea why initializing make a difference at all -- the IMO sensible behavior would be to put it in .rodata as NOBITS either way. But I'll use empty_zero_page. > >> - sg_set_buf(&sg_out[1], pad, sizeof pad); >> + sg_set_buf(&sg_out[1], zero_pad, sizeof zero_pad); > > Can you put brackets on the sizeof? Will do for v2. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Andy Lutomirski > Sent: 12 December 2016 20:53 > The driver put a constant buffer of all zeros on the stack and > pointed a scatterlist entry at it in two places. This doesn't work > with virtual stacks. Use a static 16-byte buffer of zeros instead. ... I didn't think you could dma from static data either. David -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[add some people who might know] On Tue, Dec 13, 2016 at 4:20 AM, David Laight <David.Laight@aculab.com> wrote: > From: Andy Lutomirski >> Sent: 12 December 2016 20:53 >> The driver put a constant buffer of all zeros on the stack and >> pointed a scatterlist entry at it in two places. This doesn't work >> with virtual stacks. Use a static 16-byte buffer of zeros instead. > ... > > I didn't think you could dma from static data either. According to lib/dma-debug.c, you can't dma to or from kernel text or rodata, but you can dma to or from kernel bss or data. So empty_zero_page should be okay, because it's not rodata right now. But I think this is rather silly. Joerg, Linus, etc: would it be okay to change lib/dma-debug.c to allow DMA *from* rodata? After all, rodata is ordinary memory, is backed by struct page, etc. And DMA from the zero page had better be okay because I think it happens if you mmap some zeros, don't write to them, and then direct I/O them to a device. Then I could also move empty_zero_page to rodata. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Andy Lutomirski <luto@amacapital.net> wrote:
> After all, rodata is ordinary memory, is backed by struct page, etc.
Is that actually true? I thought some arches excluded the kernel image from
the page struct array to make the array consume less memory.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 13, 2016 at 8:45 AM, David Howells <dhowells@redhat.com> wrote: > Andy Lutomirski <luto@amacapital.net> wrote: > >> After all, rodata is ordinary memory, is backed by struct page, etc. > > Is that actually true? I thought some arches excluded the kernel image from > the page struct array to make the array consume less memory. I don't know whether you're right, but that sounds a bit silly to me. This is a *tiny* amount of memory. But there's yet another snag. Alpha doesn't have empty_zero_page -- it only has ZERO_PAGE. I could do page_address(ZERO_PAGE(0))... --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Andy Lutomirski <luto@amacapital.net> wrote: > I don't know whether you're right, but that sounds a bit silly to me. > This is a *tiny* amount of memory. Assuming a 1MiB kernel image in 4K pages, that gets you back a couple of pages I think - useful if you've only got a few MiB of RAM. David -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 13, 2016 at 08:40:00AM -0800, Andy Lutomirski wrote: > But I think this is rather silly. Joerg, Linus, etc: would it be okay > to change lib/dma-debug.c to allow DMA *from* rodata? Yeah, this would be fine for DMA_TO_DEVICE mappings. At least I can't think of a reason right now to not allow it, in the end its also read-only memory for the CPU, so it can be readable by devices too. There is no danger of race conditions like on stacks or data leaks, as there is only compile-time data in rodata. Joerg -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index 17a06105ccb6..fab2fb864002 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -46,6 +46,7 @@ static const char key_format_default[] = "default"; static const char key_format_ecryptfs[] = "ecryptfs"; static unsigned int ivsize; static int blksize; +static const char zero_pad[16] = {0}; #define KEY_TRUSTED_PREFIX_LEN (sizeof (KEY_TRUSTED_PREFIX) - 1) #define KEY_USER_PREFIX_LEN (sizeof (KEY_USER_PREFIX) - 1) @@ -481,7 +482,6 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload, unsigned int encrypted_datalen; u8 iv[AES_BLOCK_SIZE]; unsigned int padlen; - char pad[16]; int ret; encrypted_datalen = roundup(epayload->decrypted_datalen, blksize); @@ -493,11 +493,10 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload, goto out; dump_decrypted_data(epayload); - memset(pad, 0, sizeof pad); sg_init_table(sg_in, 2); sg_set_buf(&sg_in[0], epayload->decrypted_data, epayload->decrypted_datalen); - sg_set_buf(&sg_in[1], pad, padlen); + sg_set_buf(&sg_in[1], zero_pad, padlen); sg_init_table(sg_out, 1); sg_set_buf(sg_out, epayload->encrypted_data, encrypted_datalen); @@ -584,7 +583,6 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload, struct skcipher_request *req; unsigned int encrypted_datalen; u8 iv[AES_BLOCK_SIZE]; - char pad[16]; int ret; encrypted_datalen = roundup(epayload->decrypted_datalen, blksize); @@ -594,13 +592,12 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload, goto out; dump_encrypted_data(epayload, encrypted_datalen); - memset(pad, 0, sizeof pad); sg_init_table(sg_in, 1); sg_init_table(sg_out, 2); sg_set_buf(sg_in, epayload->encrypted_data, encrypted_datalen); sg_set_buf(&sg_out[0], epayload->decrypted_data, epayload->decrypted_datalen); - sg_set_buf(&sg_out[1], pad, sizeof pad); + sg_set_buf(&sg_out[1], zero_pad, sizeof zero_pad); memcpy(iv, epayload->iv, sizeof(iv)); skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, iv);
The driver put a constant buffer of all zeros on the stack and pointed a scatterlist entry at it in two places. This doesn't work with virtual stacks. Use a static 16-byte buffer of zeros instead. Cc: stable@vger.kernel.org # 4.9 only Reported-by: Eric Biggers <ebiggers3@gmail.com> Signed-off-by: Andy Lutomirski <luto@kernel.org> --- security/keys/encrypted-keys/encrypted.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)