Re: small changes in aesxcbcmac.c
Alexander Nasonov wrote: > If there are no objections I'll commit the code. Ah, Christos committed it already. Less work for me then :) -- Alex
Re: small changes in aesxcbcmac.c
Alexander Nasonov wrote:
> The first change shrinks aes_xcbc_mac_init by 183 bytes on amd64
> (from 562 to 379 bytes).
> The second change avoids a comparison with an address that may
> point beyond the end of a buffer.
> The third change is stylistic.
> Alex
If there are no objections I'll commit the code.
PS I noticed some excessive memory copying (often of fixed-size blocks).
Some of them may be needed to prevent side channel attacks by measuring
execution time of cache misses. Data of the stack is more likely to be
in cache but it's not bulletproof. If we rely on this at all, buffers on
the stack should have __cacheline_aligned attribute but I don't see any
in the code.
> aes_xcbc_mac_result(u_int8_t *addr, void *vctx)
> {
> - u_char digest[AES_BLOCKSIZE];
> + u_int8_t digest[AES_BLOCKSIZE];
> aesxcbc_ctx *ctx;
> int i;
This buffer isn't actually needed. The destination addr can be passed
directly to rijndaelEncrypt() calls inside the function. I didn't
change it because it is the only array in the function and removing it
would disable ssp.
Alex
Re: small changes in aesxcbcmac.c
On Sun, Sep 25, 2016 at 10:39:17PM +, Eric Haszlakiewicz wrote: > On September 25, 2016 5:01:16 PM EDT, Alexander Nasonov > wrote: > >The first change shrinks aes_xcbc_mac_init by 183 bytes on amd64 > >(from 562 to 379 bytes). > > Do you mean it shrinks its stack usage? Or does that change to static > const vars somehow shrink the function itself? Both, the variable is put into the rodata of the kernel and the function shrinks by not having to copy things. Joerg
Re: small changes in aesxcbcmac.c
Rhialto wrote:
> On Sun 25 Sep 2016 at 22:01:16 +0100, Alexander Nasonov wrote:
> > - while (addr + AES_BLOCKSIZE < ep) {
> > + while (ep - addr > AES_BLOCKSIZE) {
>
> I think that if ep points beyond tbe buffer (apart from the
> just-past-the-end location), the subtraction is just as undefined
> behaviour as before...
If understand the code correctly, ep points to the first byte past the
end which is well defined.
Alex
signature.asc
Description: PGP signature
Re: small changes in aesxcbcmac.c
Eric Haszlakiewicz wrote: > On September 25, 2016 5:01:16 PM EDT, Alexander Nasonov > wrote: > >The first change shrinks aes_xcbc_mac_init by 183 bytes on amd64 > >(from 562 to 379 bytes). > > Do you mean it shrinks its stack usage? Or does that change to static const > vars somehow shrink the function itself? gcc copies each byte to the stack: 80532a10: c6 45 98 01 movb $0x1,-0x68(%rbp) 80532a14: c6 45 99 01 movb $0x1,-0x67(%rbp) 80532a18: c6 45 9a 01 movb $0x1,-0x66(%rbp) 80532a1c: c6 45 9b 01 movb $0x1,-0x65(%rbp) 80532a20: c6 45 9c 01 movb $0x1,-0x64(%rbp) 80532a24: c6 45 9d 01 movb $0x1,-0x63(%rbp) 80532a28: c6 45 9e 01 movb $0x1,-0x62(%rbp) 80532a2c: c6 45 9f 01 movb $0x1,-0x61(%rbp) 80532a30: c6 45 a0 01 movb $0x1,-0x60(%rbp) 80532a34: c6 45 a1 01 movb $0x1,-0x5f(%rbp) 80532a38: c6 45 a2 01 movb $0x1,-0x5e(%rbp) 80532a3c: c6 45 a3 01 movb $0x1,-0x5d(%rbp) 80532a40: c6 45 a4 01 movb $0x1,-0x5c(%rbp) 80532a44: c6 45 a5 01 movb $0x1,-0x5b(%rbp) 80532a48: c6 45 a6 01 movb $0x1,-0x5a(%rbp) 80532a4c: c6 45 a7 01 movb $0x1,-0x59(%rbp) 80532a50: c6 45 a8 02 movb $0x2,-0x58(%rbp) 80532a54: c6 45 a9 02 movb $0x2,-0x57(%rbp) 80532a58: c6 45 aa 02 movb $0x2,-0x56(%rbp) and so on. Alex
Re: small changes in aesxcbcmac.c
On September 25, 2016 5:01:16 PM EDT, Alexander Nasonov wrote: >The first change shrinks aes_xcbc_mac_init by 183 bytes on amd64 >(from 562 to 379 bytes). Do you mean it shrinks its stack usage? Or does that change to static const vars somehow shrink the function itself? Eric
Re: small changes in aesxcbcmac.c
On Sun 25 Sep 2016 at 22:01:16 +0100, Alexander Nasonov wrote:
> - while (addr + AES_BLOCKSIZE < ep) {
> + while (ep - addr > AES_BLOCKSIZE) {
I think that if ep points beyond tbe buffer (apart from the
just-past-the-end location), the subtraction is just as undefined
behaviour as before...
-Olaf.
--
___ Olaf 'Rhialto' Seibert -- Wayland: Those who don't understand X
\X/ rhialto/at/xs4all.nl-- are condemned to reinvent it. Poorly.
signature.asc
Description: PGP signature
small changes in aesxcbcmac.c
The first change shrinks aes_xcbc_mac_init by 183 bytes on amd64
(from 562 to 379 bytes).
The second change avoids a comparison with an address that may
point beyond the end of a buffer.
The third change is stylistic.
Alex
--- sys/opencrypto/aesxcbcmac.c.orig2016-09-25 21:44:25.344941650 +0100
+++ sys/opencrypto/aesxcbcmac.c 2016-09-25 13:21:43.364224984 +0100
@@ -41,9 +41,12 @@
int
aes_xcbc_mac_init(void *vctx, const u_int8_t *key, u_int16_t keylen)
{
- u_int8_t k1seed[AES_BLOCKSIZE] = { 1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1 };
- u_int8_t k2seed[AES_BLOCKSIZE] = { 2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2 };
- u_int8_t k3seed[AES_BLOCKSIZE] = { 3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3 };
+ static const u_int8_t k1seed[AES_BLOCKSIZE] =
+ { 1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1 };
+ static const u_int8_t k2seed[AES_BLOCKSIZE] =
+ { 2,2,2,2,2,2,2,2,2,2,2,2,2,2,2,2 };
+ static const u_int8_t k3seed[AES_BLOCKSIZE] =
+ { 3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3 };
u_int32_t r_ks[(RIJNDAEL_MAXNR+1)*4];
aesxcbc_ctx *ctx;
u_int8_t k1[AES_BLOCKSIZE];
@@ -98,7 +101,7 @@
ctx->buflen = 0;
}
/* due to the special processing for M[n], "=" case is not included */
- while (addr + AES_BLOCKSIZE < ep) {
+ while (ep - addr > AES_BLOCKSIZE) {
memcpy(buf, addr, AES_BLOCKSIZE);
for (i = 0; i < sizeof(buf); i++)
buf[i] ^= ctx->e[i];
@@ -115,7 +118,7 @@
void
aes_xcbc_mac_result(u_int8_t *addr, void *vctx)
{
- u_char digest[AES_BLOCKSIZE];
+ u_int8_t digest[AES_BLOCKSIZE];
aesxcbc_ctx *ctx;
int i;
