Message ID | 1507630920-9014-1-git-send-email-robert.baronescu@nxp.com (mailing list archive) |
---|---|
State | Accepted |
Delegated to: | Herbert Xu |
Headers | show |
On Tue, Oct 10, 2017 at 01:21:59PM +0300, Robert Baronescu wrote: > In case buffer length is a multiple of PAGE_SIZE, > the S/G table is incorrectly generated. > Fix this by handling buflen = k * PAGE_SIZE separately. > > Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com> > --- > crypto/tcrypt.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c > index 0022a18..bd9b66c 100644 > --- a/crypto/tcrypt.c > +++ b/crypto/tcrypt.c > @@ -221,11 +221,13 @@ static void sg_init_aead(struct scatterlist *sg, char *xbuf[XBUFSIZE], > } > > sg_init_table(sg, np + 1); > - np--; > + if (rem) > + np--; > for (k = 0; k < np; k++) > sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE); > > - sg_set_buf(&sg[k + 1], xbuf[k], rem); > + if (rem) > + sg_set_buf(&sg[k + 1], xbuf[k], rem); Sorry but I think this is still buggy because you have not moved the end-of-table marking in the rem == 0 case. Cheers,
On 11/3/2017 2:42 PM, Herbert Xu wrote: > On Tue, Oct 10, 2017 at 01:21:59PM +0300, Robert Baronescu wrote: >> In case buffer length is a multiple of PAGE_SIZE, >> the S/G table is incorrectly generated. >> Fix this by handling buflen = k * PAGE_SIZE separately. >> >> Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com> >> --- >> crypto/tcrypt.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c >> index 0022a18..bd9b66c 100644 >> --- a/crypto/tcrypt.c >> +++ b/crypto/tcrypt.c >> @@ -221,11 +221,13 @@ static void sg_init_aead(struct scatterlist *sg, char *xbuf[XBUFSIZE], >> } >> >> sg_init_table(sg, np + 1); sg_mark_end() marks sg[np]. >> - np--; >> + if (rem) >> + np--; >> for (k = 0; k < np; k++) >> sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE); In case rem == 0, last k value is np-1, thus sg[np-1+1] will be filled here with xbuf[np-1]. >> >> - sg_set_buf(&sg[k + 1], xbuf[k], rem); >> + if (rem) >> + sg_set_buf(&sg[k + 1], xbuf[k], rem); In case rem !=0, sg[np] will be filled here with xbuf[np-1]. > > Sorry but I think this is still buggy because you have not moved the > end-of-table marking in the rem == 0 case. IIUC this is correct, see above comments. Could you please take a look again? Thanks, Horia
On Thu, Nov 09, 2017 at 02:37:29PM +0000, Horia Geantă wrote: > > >> sg_init_table(sg, np + 1); > sg_mark_end() marks sg[np]. > > >> - np--; > >> + if (rem) > >> + np--; > >> for (k = 0; k < np; k++) > >> sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE); > In case rem == 0, last k value is np-1, thus sg[np-1+1] will be filled > here with xbuf[np-1]. No, if rem == 0, then the last k value is np-2. The whole point of the patch is to shrink the SG table by one entry when rem == 0. How can you shrink it without moving the end-of-table marker? Cheers,
On 11/10/2017 12:21 AM, Herbert Xu wrote: > On Thu, Nov 09, 2017 at 02:37:29PM +0000, Horia Geantă wrote: >> >>>> sg_init_table(sg, np + 1); >> sg_mark_end() marks sg[np]. >> >>>> - np--; >>>> + if (rem) >>>> + np--; >>>> for (k = 0; k < np; k++) >>>> sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE); >> In case rem == 0, last k value is np-1, thus sg[np-1+1] will be filled >> here with xbuf[np-1]. > > No, if rem == 0, then the last k value is np-2. > Notice that np-- above the for loop is done conditionally, so in the for loop k takes values in [0, np-1]. This means the for loop fills sg[1]...sg[np]. Thanks, Horia
On Fri, Nov 10, 2017 at 06:37:22AM +0000, Horia Geantă wrote: > On 11/10/2017 12:21 AM, Herbert Xu wrote: > > On Thu, Nov 09, 2017 at 02:37:29PM +0000, Horia Geantă wrote: > >> > >>>> sg_init_table(sg, np + 1); > >> sg_mark_end() marks sg[np]. > >> > >>>> - np--; > >>>> + if (rem) > >>>> + np--; > >>>> for (k = 0; k < np; k++) > >>>> sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE); > >> In case rem == 0, last k value is np-1, thus sg[np-1+1] will be filled > >> here with xbuf[np-1]. > > > > No, if rem == 0, then the last k value is np-2. > > > Notice that np-- above the for loop is done conditionally, so in the for > loop k takes values in [0, np-1]. > This means the for loop fills sg[1]...sg[np]. I must be missing something. In the case rem == 0, let's say the original value of np is npo. Then at the start of the loop, np = npo - 1, and at the last iteration, k = npo - 2, so we do sg_set_buf(&sg[npo - 1], xbuf[npo - 2], PAGE_SIZE); While the sg_init_table call sets the end-of-table at sg_init_table(sg, npo + 1); I can't see how this can be right. Cheers,
On 11/10/2017 9:43 AM, Herbert Xu wrote: > On Fri, Nov 10, 2017 at 06:37:22AM +0000, Horia Geantă wrote: >> On 11/10/2017 12:21 AM, Herbert Xu wrote: >>> On Thu, Nov 09, 2017 at 02:37:29PM +0000, Horia Geantă wrote: >>>> >>>>>> sg_init_table(sg, np + 1); >>>> sg_mark_end() marks sg[np]. >>>> >>>>>> - np--; >>>>>> + if (rem) >>>>>> + np--; >>>>>> for (k = 0; k < np; k++) >>>>>> sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE); >>>> In case rem == 0, last k value is np-1, thus sg[np-1+1] will be filled >>>> here with xbuf[np-1]. >>> >>> No, if rem == 0, then the last k value is np-2. >>> >> Notice that np-- above the for loop is done conditionally, so in the for >> loop k takes values in [0, np-1]. >> This means the for loop fills sg[1]...sg[np]. > > I must be missing something. In the case rem == 0, let's say > the original value of np is npo. Then at the start of the loop, > np = npo - 1, and at the last iteration, k = npo - 2, so we do IIUC at the start of the loop np = npo (and not npo - 1), since np is no longer decremented in the rem == 0 case: - np--; + if (rem) + np--; > > sg_set_buf(&sg[npo - 1], xbuf[npo - 2], PAGE_SIZE); > and accordingly last iteration is for k = npo - 1: sg_set_buf(&sg[npo], xbuf[npo - 1], PAGE_SIZE); > While the sg_init_table call sets the end-of-table at > > sg_init_table(sg, npo + 1); > while this marks sg[npo] as last SG table entry. Thanks, Horia
On Fri, Nov 10, 2017 at 09:17:33AM +0000, Horia Geantă wrote: > > > I must be missing something. In the case rem == 0, let's say > > the original value of np is npo. Then at the start of the loop, > > np = npo - 1, and at the last iteration, k = npo - 2, so we do > IIUC at the start of the loop np = npo (and not npo - 1), since np is no > longer decremented in the rem == 0 case: > - np--; > + if (rem) > + np--; > > > > > sg_set_buf(&sg[npo - 1], xbuf[npo - 2], PAGE_SIZE); > > > and accordingly last iteration is for k = npo - 1: > sg_set_buf(&sg[npo], xbuf[npo - 1], PAGE_SIZE); > > > While the sg_init_table call sets the end-of-table at > > > > sg_init_table(sg, npo + 1); > > > while this marks sg[npo] as last SG table entry. OK, we're both sort of right. You're correct that this generates a valid SG list in that the number of entries match the end-of-table marking. But the thing that prompted to check this patch in the first place is the semantics behind it. For the case rem == 0, it means that buflen is a multiple of PAGE_SIZE. In that case, the code with your patch will create an SG list that's one page longer than buflen. AFAICS it should be harmless but it would still be better if we can generate an SG list that's exactly buflen long rather than one page longer. Cheers,
On 11/10/2017 1:23 PM, Herbert Xu wrote: > On Fri, Nov 10, 2017 at 09:17:33AM +0000, Horia Geantă wrote: >> >>> I must be missing something. In the case rem == 0, let's say >>> the original value of np is npo. Then at the start of the loop, >>> np = npo - 1, and at the last iteration, k = npo - 2, so we do >> IIUC at the start of the loop np = npo (and not npo - 1), since np is no >> longer decremented in the rem == 0 case: >> - np--; >> + if (rem) >> + np--; >> >>> >>> sg_set_buf(&sg[npo - 1], xbuf[npo - 2], PAGE_SIZE); >>> >> and accordingly last iteration is for k = npo - 1: >> sg_set_buf(&sg[npo], xbuf[npo - 1], PAGE_SIZE); >> >>> While the sg_init_table call sets the end-of-table at >>> >>> sg_init_table(sg, npo + 1); >>> >> while this marks sg[npo] as last SG table entry. > > OK, we're both sort of right. You're correct that this generates > a valid SG list in that the number of entries match the end-of-table > marking. > > But the thing that prompted to check this patch in the first place > is the semantics behind it. For the case rem == 0, it means that > buflen is a multiple of PAGE_SIZE. In that case, the code with > your patch will create an SG list that's one page longer than > buflen. > SG table always has 1 entry more than what's needed strictly for input data. Let's say buflen = npo * PAGE_SIZE. SG table generated by the code will have npo + 1 entries: -sg[0] - (1 entry) reserved for associated data, filled outside sg_init_aead() -sg[1]..sg[npo] (npo entries) - input data, entries pointing to xbuf[0]..xbuf[npo-1] Horia
Hi, On 10/10/2017 01:21 PM, Robert Baronescu wrote: > In case buffer length is a multiple of PAGE_SIZE, > the S/G table is incorrectly generated. > Fix this by handling buflen = k * PAGE_SIZE separately. > > Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com> > --- > crypto/tcrypt.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) This patch fixes the segmentation fault listed below. The NULL dereference can be seen starting with: 7aacbfc crypto: tcrypt - fix buffer lengths in test_aead_speed() Cheers, ta # insmod tcrypt.ko mode=212 testing speed of rfc4309(ccm(aes)) (rfc4309(ccm_base(ctr(aes-generic),cbcmac(aes-generic)))) encryption test 0 (152 bit key, 16 byte blocks): 1 operation in 0 cycles (16 bytes) test 1 (152 bit key, 64 byte blocks): 1 operation in 0 cycles (64 bytes) test 2 (152 bit key, 256 byte blocks): 1 operation in 0 cycles (256 bytes) test 3 (152 bit key, 512 byte blocks): 1 operation in 0 cycles (512 bytes) test 4 (152 bit key, 1024 byte blocks): 1 operation in 0 cycles (1024 bytes) test 5 (152 bit key, 2048 byte blocks): 1 operation in 0 cycles (2048 bytes) test 6 (152 bit key, 4096 byte blocks): Unable to handle kernel NULL pointer dereference at virtual address 00000004 pgd = deee0000 [00000004] *pgd=3f6b8831, *pte=00000000, *ppte=00000000 Internal error: Oops: 17 [#1] ARM Modules linked in: tcrypt(+) CPU: 0 PID: 795 Comm: insmod Not tainted 4.14.0-rc3+ #15 Hardware name: Atmel SAMA5 task: def4d000 task.stack: def4a000 PC is at scatterwalk_copychunks+0x14c/0x18c LR is at scatterwalk_copychunks+0x144/0x18c pc : [<c02c2d84>] lr : [<c02c2d7c>] psr: 20000013 sp : def4bbf8 ip : 00000000 fp : def4bcb4 r10: c02d1e5c r9 : 00000000 r8 : def4a000 r7 : defd0090 r6 : def4bc58 r5 : 00000010 r4 : 00000000 r3 : dffe71e2 r2 : def4d000 r1 : 00000000 r0 : 00000000 Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c53c7d Table: 3eee0059 DAC: 00000051 Process insmod (pid: 795, stack limit = 0xdef4a208) Stack: (0xdef4bbf8 to 0xdef4c000) bbe0: def4bc48 00000010 bc00: def4bcbc ffffffff 00000010 00000000 c02d1e5c c02c47f0 00000010 def4bc28 bc20: deefe110 00000000 deefe200 def11800 c02d1e5c c02cc178 000000e7 def4bc38 bc40: 00000010 def4bcbc dffd8fc0 defd0090 dffd8fc0 defd0080 00000000 00000000 bc60: 00001000 def7e2a0 00000000 00001000 00000000 defd0080 deefe200 00000010 bc80: 00000000 00000010 00000001 00000000 00000000 c02cc0bc 00000000 ded1a4c0 bca0: 00001000 deefe200 deefe0c0 deefe134 deefe164 c02c509c 00001000 deda5280 bcc0: deefe200 00000400 deefe100 c02cec9c def4bd70 deefe000 00000000 deefe000 bce0: 00000000 00000004 00000000 def7e200 bf007144 ded19300 00000000 bf001950 bd00: 014000c0 bf007234 00000000 00000010 bf0075c0 def7e290 deda7a80 00000006 bd20: c0a4bd38 00001000 00000000 ded19300 bf007140 ded19340 00000000 defd0f00 bd40: 00000000 def4bd44 def4bd44 c0176ea4 df60f000 def5c000 def5e000 deff1000 bd60: df4a5000 df651000 df648000 df646000 deebe000 dee59000 deeae000 defd1000 bd80: deda0000 defd3000 de806000 def82000 def63000 def78000 deec7000 deeff000 bda0: deeb9000 deef2000 deeba000 deebd000 00000000 00000000 00000004 bf0075c0 bdc0: bf007440 defd0f00 bf007488 00000001 2102f11c bf005238 df4ac000 000075c0 bde0: 00000003 bf0075c0 bf007440 bf0075c0 00000004 bf0075c0 bf007440 defd0f00 be00: bf007488 bf00a054 bf007440 bf00a000 00000000 c01018e8 00000000 ded17780 be20: df4ac000 c0a3a72c df420000 c0844a4c c07df704 c01a5054 bf007488 c0684d38 be40: 00000012 deda7440 defd0f08 a0000013 deda7640 e0a7e000 00000001 defd0f00 be60: bf007440 defd0f08 deda7640 defd0f00 bf007488 c016203c bf007488 00000001 be80: def4bf50 defd0f08 00000001 c0161390 bf00744c 00007fff bf007440 c015ea8c bea0: 00000000 bf007590 00000578 bf007528 c0844c7c c07018f0 c01b1060 bf000000 bec0: 0000dcfb 0000dcfb 00000000 00000000 00000000 00000000 00000000 00000000 bee0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 bf00: 00000000 00000000 7fffffff 00000000 00000003 00099008 0000017b c0107964 bf20: def4a000 00000000 00000000 c0161a68 7fffffff 00000000 00000003 a0000013 bf40: dedd1c00 e0a7e000 0000dcfb 00000000 e0a83d03 e0a7e000 0000dcfb e0a85238 bf60: e0a850dd e0a8b258 00008000 000081d0 00000000 00000000 00000000 00002e84 bf80: 00000021 00000022 00000019 00000000 00000013 00000000 00099008 bebd1f45 bfa0: 00000003 c01077a0 00099008 bebd1f45 00000003 00099008 00000000 bebd1f45 bfc0: 00099008 bebd1f45 00000003 0000017b bebd1f45 00000000 00000000 00000000 bfe0: bebd1ca8 bebd1c98 0001f99d b6f3f2c4 80000030 00000003 00000000 00000000 [<c02c2d84>] (scatterwalk_copychunks) from [<c02c47f0>] (blkcipher_walk_next+0x3a0/0x44c) [<c02c47f0>] (blkcipher_walk_next) from [<c02cc178>] (crypto_ctr_crypt+0xbc/0x1cc) [<c02cc178>] (crypto_ctr_crypt) from [<c02c509c>] (skcipher_encrypt_blkcipher+0x44/0x4c) [<c02c509c>] (skcipher_encrypt_blkcipher) from [<c02cec9c>] (crypto_ccm_encrypt+0xc8/0xf8) [<c02cec9c>] (crypto_ccm_encrypt) from [<bf001950>] (test_aead_speed.constprop.2+0x3e8/0x5a8 [tcrypt]) [<bf001950>] (test_aead_speed.constprop.2 [tcrypt]) from [<bf005238>] (do_test+0x3728/0x3e88 [tcrypt]) [<bf005238>] (do_test [tcrypt]) from [<bf00a054>] (tcrypt_mod_init+0x54/0x1000 [tcrypt]) [<bf00a054>] (tcrypt_mod_init [tcrypt]) from [<c01018e8>] (do_one_initcall+0x40/0x16c) [<c01018e8>] (do_one_initcall) from [<c016203c>] (do_init_module+0x60/0x1d8) [<c016203c>] (do_init_module) from [<c0161390>] (load_module+0x1c4c/0x214c) [<c0161390>] (load_module) from [<c0161a68>] (SyS_finit_module+0x8c/0x9c) [<c0161a68>] (SyS_finit_module) from [<c01077a0>] (ret_fast_syscall+0x0/0x48) Code: e1a00001 eb00da5e e5860000 e1a01000 (e590c004) ---[ end trace d97c437cd566fdf4 ]--- Segmentation fault
Hi, On 11/12/2017 06:26 PM, Horia Geantă wrote: > -sg[0] - (1 entry) reserved for associated data, filled outside > sg_init_aead() Let's fill the sg[0] with aad inside sg_init_aead()! Cheers, ta
On 11/13/2017 8:24 PM, Tudor Ambarus wrote: > Hi, > > On 10/10/2017 01:21 PM, Robert Baronescu wrote: >> In case buffer length is a multiple of PAGE_SIZE, >> the S/G table is incorrectly generated. >> Fix this by handling buflen = k * PAGE_SIZE separately. >> >> Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com> >> --- >> crypto/tcrypt.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) > > This patch fixes the segmentation fault listed below. The NULL > dereference can be seen starting with: > 7aacbfc crypto: tcrypt - fix buffer lengths in test_aead_speed() > Right, the order of the two fixes is unfortunate in the sense that first fix (commit 7aacbfc) uncovers the issue you mention. Thanks, Horia
On 11/13/2017 8:28 PM, Tudor Ambarus wrote: > Hi, > > On 11/12/2017 06:26 PM, Horia Geantă wrote: > >> -sg[0] - (1 entry) reserved for associated data, filled outside >> sg_init_aead() > > Let's fill the sg[0] with aad inside sg_init_aead()! > This could be done, however I would not mix fixes with improvements. Thanks, Horia
On Sun, Nov 12, 2017 at 04:26:39PM +0000, Horia Geantă wrote: > > SG table always has 1 entry more than what's needed strictly for input data. > > Let's say buflen = npo * PAGE_SIZE. > SG table generated by the code will have npo + 1 entries: > -sg[0] - (1 entry) reserved for associated data, filled outside > sg_init_aead() > -sg[1]..sg[npo] (npo entries) - input data, entries pointing to > xbuf[0]..xbuf[npo-1] You are right. I will apply the patch. Thanks!
On Tue, Oct 10, 2017 at 01:21:59PM +0300, Robert Baronescu wrote: > In case buffer length is a multiple of PAGE_SIZE, > the S/G table is incorrectly generated. > Fix this by handling buflen = k * PAGE_SIZE separately. > > Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com> Patch applied. Thanks.
On 11/29/2017 8:28 AM, Herbert Xu wrote: > On Tue, Oct 10, 2017 at 01:21:59PM +0300, Robert Baronescu wrote: >> In case buffer length is a multiple of PAGE_SIZE, >> the S/G table is incorrectly generated. >> Fix this by handling buflen = k * PAGE_SIZE separately. >> >> Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com> > > Patch applied. Thanks. > Thanks Herbert. Considering this fixes the crash reported by Tudor: https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg29172.html I think it should be merged in this release cycle (v4.15). Horia
On Wed, Nov 29, 2017 at 08:57:33AM +0000, Horia Geantă wrote: > > Considering this fixes the crash reported by Tudor: > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg29172.html > I think it should be merged in this release cycle (v4.15). This only affects tcrypt right? Since tcrypt is merely a debugging interface I don't think it's that critical. Cheers,
diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c index 0022a18..bd9b66c 100644 --- a/crypto/tcrypt.c +++ b/crypto/tcrypt.c @@ -221,11 +221,13 @@ static void sg_init_aead(struct scatterlist *sg, char *xbuf[XBUFSIZE], } sg_init_table(sg, np + 1); - np--; + if (rem) + np--; for (k = 0; k < np; k++) sg_set_buf(&sg[k + 1], xbuf[k], PAGE_SIZE); - sg_set_buf(&sg[k + 1], xbuf[k], rem); + if (rem) + sg_set_buf(&sg[k + 1], xbuf[k], rem); } static void test_aead_speed(const char *algo, int enc, unsigned int secs,
In case buffer length is a multiple of PAGE_SIZE, the S/G table is incorrectly generated. Fix this by handling buflen = k * PAGE_SIZE separately. Signed-off-by: Robert Baronescu <robert.baronescu@nxp.com> --- crypto/tcrypt.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)