linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c
@ 2021-07-05  5:09 Jarkko Sakkinen
  2021-07-05  5:10 ` Jarkko Sakkinen
  2021-07-23 19:53 ` Shuah Khan
  0 siblings, 2 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2021-07-05  5:09 UTC (permalink / raw)
  To: Shuah Khan
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Borislav Petkov,
	Tianjia Zhang, Jarkko Sakkinen, Dave Hansen, linux-kernel

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--;
+	}
+}
+
 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);
-- 
2.32.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c
  2021-07-05  5:09 [PATCH v2] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c Jarkko Sakkinen
@ 2021-07-05  5:10 ` Jarkko Sakkinen
  2021-07-23 19:53 ` Shuah Khan
  1 sibling, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2021-07-05  5:10 UTC (permalink / raw)
  To: Shuah Khan
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Borislav Petkov,
	Tianjia Zhang, Dave Hansen, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c
  2021-07-05  5:09 [PATCH v2] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c Jarkko Sakkinen
  2021-07-05  5:10 ` Jarkko Sakkinen
@ 2021-07-23 19:53 ` Shuah Khan
  2021-07-27  3:12   ` Jarkko Sakkinen
  1 sibling, 1 reply; 6+ messages in thread
From: Shuah Khan @ 2021-07-23 19:53 UTC (permalink / raw)
  To: Jarkko Sakkinen, Shuah Khan
  Cc: linux-kselftest, linux-sgx, Reinette Chatre, Borislav Petkov,
	Tianjia Zhang, Dave Hansen, linux-kernel, Shuah Khan

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c
  2021-07-23 19:53 ` Shuah Khan
@ 2021-07-27  3:12   ` Jarkko Sakkinen
  2021-07-29 21:33     ` Shuah Khan
  0 siblings, 1 reply; 6+ messages in thread
From: Jarkko Sakkinen @ 2021-07-27  3:12 UTC (permalink / raw)
  To: Shuah Khan
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Reinette Chatre,
	Borislav Petkov, Tianjia Zhang, Dave Hansen, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c
  2021-07-27  3:12   ` Jarkko Sakkinen
@ 2021-07-29 21:33     ` Shuah Khan
  2021-07-30  1:03       ` Jarkko Sakkinen
  0 siblings, 1 reply; 6+ messages in thread
From: Shuah Khan @ 2021-07-29 21:33 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Reinette Chatre,
	Borislav Petkov, Tianjia Zhang, Dave Hansen, linux-kernel,
	Shuah Khan

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c
  2021-07-29 21:33     ` Shuah Khan
@ 2021-07-30  1:03       ` Jarkko Sakkinen
  0 siblings, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2021-07-30  1:03 UTC (permalink / raw)
  To: Shuah Khan
  Cc: Shuah Khan, linux-kselftest, linux-sgx, Reinette Chatre,
	Borislav Petkov, Tianjia Zhang, Dave Hansen, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-07-30  1:03 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-05  5:09 [PATCH v2] selftests/sgx: Fix Q1 and Q2 calculation in sigstruct.c Jarkko Sakkinen
2021-07-05  5:10 ` Jarkko Sakkinen
2021-07-23 19:53 ` Shuah Khan
2021-07-27  3:12   ` Jarkko Sakkinen
2021-07-29 21:33     ` Shuah Khan
2021-07-30  1:03       ` Jarkko Sakkinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).