| Message ID | 20210705050922.63710-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 567c39047dbee341244fe3bf79fea24ee0897ff9 |
| Headers | show |
| Series | [v2] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c | expand |
On Mon, Jul 05, 2021 at 08:09:21AM +0300, Jarkko Sakkinen wrote: > From: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > > Q1 and Q2 are numbers with *maximum* length of 384 bytes. If the calculated > length of Q1 and Q2 is less than 384 bytes, things will go wrong. > > E.g. if Q2 is 383 bytes, then > > 1. The bytes of q2 are copied to sigstruct->q2 in calc_q1q2(). > 2. The entire sigstruct->q2 is reversed, which results it being > 256 * Q2, given that the last byte of sigstruct->q2 is added > to before the bytes given by calc_q1q2(). > > Either change in key or measurement can trigger the bug. E.g. an unmeasured > heap could cause a devastating change in Q1 or Q2. > > Reverse exactly the bytes of Q1 and Q2 in calc_q1q2() before returning to > the caller. > > Fixes: dedde2634570 ("selftests/sgx: Trigger the reclaimer in the selftests") Oops :-( Was meant to be Fixes: 2adcba79e69d ("selftests/x86: Add a selftest for SGX") /Jarkko
On 7/4/21 11:09 PM, Jarkko Sakkinen wrote: > From: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > > Q1 and Q2 are numbers with *maximum* length of 384 bytes. If the calculated > length of Q1 and Q2 is less than 384 bytes, things will go wrong. > > E.g. if Q2 is 383 bytes, then > > 1. The bytes of q2 are copied to sigstruct->q2 in calc_q1q2(). > 2. The entire sigstruct->q2 is reversed, which results it being > 256 * Q2, given that the last byte of sigstruct->q2 is added > to before the bytes given by calc_q1q2(). > > Either change in key or measurement can trigger the bug. E.g. an unmeasured > heap could cause a devastating change in Q1 or Q2. > > Reverse exactly the bytes of Q1 and Q2 in calc_q1q2() before returning to > the caller. > > Fixes: dedde2634570 ("selftests/sgx: Trigger the reclaimer in the selftests") > Link: https://lore.kernel.org/linux-sgx/20210301051836.30738-1-tianjia.zhang@linux.alibaba.com/ > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > --- > The original patch did a bad job explaining the code change but it > turned out making sense. I wrote a new description. > > v2: > - Added a fixes tag. > tools/testing/selftests/sgx/sigstruct.c | 41 +++++++++++++------------ > 1 file changed, 21 insertions(+), 20 deletions(-) > > diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c > index dee7a3d6c5a5..92bbc5a15c39 100644 > --- a/tools/testing/selftests/sgx/sigstruct.c > +++ b/tools/testing/selftests/sgx/sigstruct.c > @@ -55,10 +55,27 @@ static bool alloc_q1q2_ctx(const uint8_t *s, const uint8_t *m, > return true; > } > > +static void reverse_bytes(void *data, int length) > +{ > + int i = 0; > + int j = length - 1; > + uint8_t temp; > + uint8_t *ptr = data; > + > + while (i < j) { > + temp = ptr[i]; > + ptr[i] = ptr[j]; > + ptr[j] = temp; > + i++; > + j--; > + } > +} I was just about apply this one and noticed this reverse_bytes(). Aren't there byteswap functions you could call instead of writing your own? thanks, -- Shuah
On Fri, Jul 23, 2021 at 01:53:06PM -0600, Shuah Khan wrote: > On 7/4/21 11:09 PM, Jarkko Sakkinen wrote: > > From: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > > > > Q1 and Q2 are numbers with *maximum* length of 384 bytes. If the calculated > > length of Q1 and Q2 is less than 384 bytes, things will go wrong. > > > > E.g. if Q2 is 383 bytes, then > > > > 1. The bytes of q2 are copied to sigstruct->q2 in calc_q1q2(). > > 2. The entire sigstruct->q2 is reversed, which results it being > > 256 * Q2, given that the last byte of sigstruct->q2 is added > > to before the bytes given by calc_q1q2(). > > > > Either change in key or measurement can trigger the bug. E.g. an unmeasured > > heap could cause a devastating change in Q1 or Q2. > > > > Reverse exactly the bytes of Q1 and Q2 in calc_q1q2() before returning to > > the caller. > > > > Fixes: dedde2634570 ("selftests/sgx: Trigger the reclaimer in the selftests") > > Link: https://lore.kernel.org/linux-sgx/20210301051836.30738-1-tianjia.zhang@linux.alibaba.com/ > > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > --- > > The original patch did a bad job explaining the code change but it > > turned out making sense. I wrote a new description. > > > > v2: > > - Added a fixes tag. > > tools/testing/selftests/sgx/sigstruct.c | 41 +++++++++++++------------ > > 1 file changed, 21 insertions(+), 20 deletions(-) > > > > diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c > > index dee7a3d6c5a5..92bbc5a15c39 100644 > > --- a/tools/testing/selftests/sgx/sigstruct.c > > +++ b/tools/testing/selftests/sgx/sigstruct.c > > @@ -55,10 +55,27 @@ static bool alloc_q1q2_ctx(const uint8_t *s, const uint8_t *m, > > return true; > > } > > +static void reverse_bytes(void *data, int length) > > +{ > > + int i = 0; > > + int j = length - 1; > > + uint8_t temp; > > + uint8_t *ptr = data; > > + > > + while (i < j) { > > + temp = ptr[i]; > > + ptr[i] = ptr[j]; > > + ptr[j] = temp; > > + i++; > > + j--; > > + } > > +} > > I was just about apply this one and noticed this reverse_bytes(). > Aren't there byteswap functions you could call instead of writing > your own? Sorry for latency, just came from two week leave. glibc does provide bswap for 16, 32, 64 bit numbers but nothing better. I have no idea if libssl has such function. Since the test code already uses this function, and it works, and it's not a newly added function in this patch, I would consider keeping it. > thanks, > -- Shuah /Jarkko
On 7/26/21 9:12 PM, Jarkko Sakkinen wrote: > On Fri, Jul 23, 2021 at 01:53:06PM -0600, Shuah Khan wrote: >> On 7/4/21 11:09 PM, Jarkko Sakkinen wrote: >>> From: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> >>> >>> Q1 and Q2 are numbers with *maximum* length of 384 bytes. If the calculated >>> length of Q1 and Q2 is less than 384 bytes, things will go wrong. >>> >>> E.g. if Q2 is 383 bytes, then >>> >>> 1. The bytes of q2 are copied to sigstruct->q2 in calc_q1q2(). >>> 2. The entire sigstruct->q2 is reversed, which results it being >>> 256 * Q2, given that the last byte of sigstruct->q2 is added >>> to before the bytes given by calc_q1q2(). >>> >>> Either change in key or measurement can trigger the bug. E.g. an unmeasured >>> heap could cause a devastating change in Q1 or Q2. >>> >>> Reverse exactly the bytes of Q1 and Q2 in calc_q1q2() before returning to >>> the caller. >>> >>> Fixes: dedde2634570 ("selftests/sgx: Trigger the reclaimer in the selftests") >>> Link: https://lore.kernel.org/linux-sgx/20210301051836.30738-1-tianjia.zhang@linux.alibaba.com/ >>> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> >>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> >>> --- >>> The original patch did a bad job explaining the code change but it >>> turned out making sense. I wrote a new description. >>> >>> v2: >>> - Added a fixes tag. >>> tools/testing/selftests/sgx/sigstruct.c | 41 +++++++++++++------------ >>> 1 file changed, 21 insertions(+), 20 deletions(-) >>> >>> diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c >>> index dee7a3d6c5a5..92bbc5a15c39 100644 >>> --- a/tools/testing/selftests/sgx/sigstruct.c >>> +++ b/tools/testing/selftests/sgx/sigstruct.c >>> @@ -55,10 +55,27 @@ static bool alloc_q1q2_ctx(const uint8_t *s, const uint8_t *m, >>> return true; >>> } >>> +static void reverse_bytes(void *data, int length) >>> +{ >>> + int i = 0; >>> + int j = length - 1; >>> + uint8_t temp; >>> + uint8_t *ptr = data; >>> + >>> + while (i < j) { >>> + temp = ptr[i]; >>> + ptr[i] = ptr[j]; >>> + ptr[j] = temp; >>> + i++; >>> + j--; >>> + } >>> +} >> >> I was just about apply this one and noticed this reverse_bytes(). >> Aren't there byteswap functions you could call instead of writing >> your own? > > Sorry for latency, just came from two week leave. > > glibc does provide bswap for 16, 32, 64 bit numbers but nothing better. > > I have no idea if libssl has such function. Since the test code already > uses this function, and it works, and it's not a newly added function in > this patch, I would consider keeping it. > I will queue this up since it is fixing an important problem. Let's look into if this can be replaced with a lib call when you do cleanups perhaps for the next release. thanks, -- Shuah
On Thu, Jul 29, 2021 at 03:33:10PM -0600, Shuah Khan wrote: > On 7/26/21 9:12 PM, Jarkko Sakkinen wrote: > > On Fri, Jul 23, 2021 at 01:53:06PM -0600, Shuah Khan wrote: > > > On 7/4/21 11:09 PM, Jarkko Sakkinen wrote: > > > > From: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > > > > > > > > Q1 and Q2 are numbers with *maximum* length of 384 bytes. If the calculated > > > > length of Q1 and Q2 is less than 384 bytes, things will go wrong. > > > > > > > > E.g. if Q2 is 383 bytes, then > > > > > > > > 1. The bytes of q2 are copied to sigstruct->q2 in calc_q1q2(). > > > > 2. The entire sigstruct->q2 is reversed, which results it being > > > > 256 * Q2, given that the last byte of sigstruct->q2 is added > > > > to before the bytes given by calc_q1q2(). > > > > > > > > Either change in key or measurement can trigger the bug. E.g. an unmeasured > > > > heap could cause a devastating change in Q1 or Q2. > > > > > > > > Reverse exactly the bytes of Q1 and Q2 in calc_q1q2() before returning to > > > > the caller. > > > > > > > > Fixes: dedde2634570 ("selftests/sgx: Trigger the reclaimer in the selftests") > > > > Link: https://lore.kernel.org/linux-sgx/20210301051836.30738-1-tianjia.zhang@linux.alibaba.com/ > > > > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > --- > > > > The original patch did a bad job explaining the code change but it > > > > turned out making sense. I wrote a new description. > > > > > > > > v2: > > > > - Added a fixes tag. > > > > tools/testing/selftests/sgx/sigstruct.c | 41 +++++++++++++------------ > > > > 1 file changed, 21 insertions(+), 20 deletions(-) > > > > > > > > diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c > > > > index dee7a3d6c5a5..92bbc5a15c39 100644 > > > > --- a/tools/testing/selftests/sgx/sigstruct.c > > > > +++ b/tools/testing/selftests/sgx/sigstruct.c > > > > @@ -55,10 +55,27 @@ static bool alloc_q1q2_ctx(const uint8_t *s, const uint8_t *m, > > > > return true; > > > > } > > > > +static void reverse_bytes(void *data, int length) > > > > +{ > > > > + int i = 0; > > > > + int j = length - 1; > > > > + uint8_t temp; > > > > + uint8_t *ptr = data; > > > > + > > > > + while (i < j) { > > > > + temp = ptr[i]; > > > > + ptr[i] = ptr[j]; > > > > + ptr[j] = temp; > > > > + i++; > > > > + j--; > > > > + } > > > > +} > > > > > > I was just about apply this one and noticed this reverse_bytes(). > > > Aren't there byteswap functions you could call instead of writing > > > your own? > > > > Sorry for latency, just came from two week leave. > > > > glibc does provide bswap for 16, 32, 64 bit numbers but nothing better. > > I have no idea if libssl has such function. Since the test code already > > uses this function, and it works, and it's not a newly added function in > > this patch, I would consider keeping it. > > I will queue this up since it is fixing an important problem. > Let's look into if this can be replaced with a lib call when > you do cleanups perhaps for the next release. Thank you. /Jarkko
diff --git a/tools/testing/selftests/sgx/sigstruct.c b/tools/testing/selftests/sgx/sigstruct.c index dee7a3d6c5a5..92bbc5a15c39 100644 --- a/tools/testing/selftests/sgx/sigstruct.c +++ b/tools/testing/selftests/sgx/sigstruct.c @@ -55,10 +55,27 @@ static bool alloc_q1q2_ctx(const uint8_t *s, const uint8_t *m, return true; } +static void reverse_bytes(void *data, int length) +{ + int i = 0; + int j = length - 1; + uint8_t temp; + uint8_t *ptr = data; + + while (i < j) { + temp = ptr[i]; + ptr[i] = ptr[j]; + ptr[j] = temp; + i++; + j--; + } +} + static bool calc_q1q2(const uint8_t *s, const uint8_t *m, uint8_t *q1, uint8_t *q2) { struct q1q2_ctx ctx; + int len; if (!alloc_q1q2_ctx(s, m, &ctx)) { fprintf(stderr, "Not enough memory for Q1Q2 calculation\n"); @@ -89,8 +106,10 @@ static bool calc_q1q2(const uint8_t *s, const uint8_t *m, uint8_t *q1, goto out; } - BN_bn2bin(ctx.q1, q1); - BN_bn2bin(ctx.q2, q2); + len = BN_bn2bin(ctx.q1, q1); + reverse_bytes(q1, len); + len = BN_bn2bin(ctx.q2, q2); + reverse_bytes(q2, len); free_q1q2_ctx(&ctx); return true; @@ -152,22 +171,6 @@ static RSA *gen_sign_key(void) return key; } -static void reverse_bytes(void *data, int length) -{ - int i = 0; - int j = length - 1; - uint8_t temp; - uint8_t *ptr = data; - - while (i < j) { - temp = ptr[i]; - ptr[i] = ptr[j]; - ptr[j] = temp; - i++; - j--; - } -} - enum mrtags { MRECREATE = 0x0045544145524345, MREADD = 0x0000000044444145, @@ -367,8 +370,6 @@ bool encl_measure(struct encl *encl) /* BE -> LE */ reverse_bytes(sigstruct->signature, SGX_MODULUS_SIZE); reverse_bytes(sigstruct->modulus, SGX_MODULUS_SIZE); - reverse_bytes(sigstruct->q1, SGX_MODULUS_SIZE); - reverse_bytes(sigstruct->q2, SGX_MODULUS_SIZE); EVP_MD_CTX_destroy(ctx); RSA_free(key);