All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] crypto: ccp - Limit memory allocation in SEV_GET_ID2 ioctl
@ 2022-12-14 20:20 Peter Gonda
  2022-12-15 10:05 ` Herbert Xu
  0 siblings, 1 reply; 12+ messages in thread
From: Peter Gonda @ 2022-12-14 20:20 UTC (permalink / raw)
  To: herbert
  Cc: Peter Gonda, Andy Nguyen, David Rientjes, stable, linux-kernel,
	linux-crypto, John Allen, Thomas . Lendacky

Currently userspace can ask for any uint32 size allocation for the
SEV_GET_ID2. Limit this allocation size to the max physically
contiguously allocation: MAX_ORDER.

Reported-by: Andy Nguyen <theflow@google.com>
Suggested-by: David Rientjes <rientjes@google.com>
Signed-off-by: Peter Gonda <pgonda@google.com>
Cc: stable@vger.kernel.org
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: linux-kernel@vger.kernel.org
Cc: linux-crypto@vger.kernel.org
Cc: John Allen <john.allen@amd.com>
Cc: Thomas.Lendacky <thomas.lendacky@amd.com>

---
 drivers/crypto/ccp/sev-dev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 06fc7156c04f..5c16c4406764 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -878,6 +878,10 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
 	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
 		return -EFAULT;
 
+	/* Max length that can be allocated physically contiguously */
+	if (get_order(input.length) >= MAX_ORDER)
+		return -ENOMEM;
+
 	input_address = (void __user *)input.address;
 
 	if (input.address && input.length) {
-- 
2.39.0.rc1.256.g54fd8350bd-goog


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

* Re: [PATCH] crypto: ccp - Limit memory allocation in SEV_GET_ID2 ioctl
  2022-12-14 20:20 [PATCH] crypto: ccp - Limit memory allocation in SEV_GET_ID2 ioctl Peter Gonda
@ 2022-12-15 10:05 ` Herbert Xu
  2022-12-28  1:42   ` David Rientjes
  0 siblings, 1 reply; 12+ messages in thread
From: Herbert Xu @ 2022-12-15 10:05 UTC (permalink / raw)
  To: Peter Gonda
  Cc: Andy Nguyen, David Rientjes, stable, linux-kernel, linux-crypto,
	John Allen, Thomas . Lendacky

On Wed, Dec 14, 2022 at 12:20:46PM -0800, Peter Gonda wrote:
> Currently userspace can ask for any uint32 size allocation for the
> SEV_GET_ID2. Limit this allocation size to the max physically
> contiguously allocation: MAX_ORDER.

This is just to silence the alloc_pages warning, right? If so
how about adding __GFP_NOWARN instead?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

* Re: [PATCH] crypto: ccp - Limit memory allocation in SEV_GET_ID2 ioctl
  2022-12-15 10:05 ` Herbert Xu
@ 2022-12-28  1:42   ` David Rientjes
  2022-12-28  8:49     ` Herbert Xu
  0 siblings, 1 reply; 12+ messages in thread
From: David Rientjes @ 2022-12-28  1:42 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Peter Gonda, Andy Nguyen, stable, linux-kernel, linux-crypto,
	John Allen, Thomas . Lendacky

On Thu, 15 Dec 2022, Herbert Xu wrote:

> On Wed, Dec 14, 2022 at 12:20:46PM -0800, Peter Gonda wrote:
> > Currently userspace can ask for any uint32 size allocation for the
> > SEV_GET_ID2. Limit this allocation size to the max physically
> > contiguously allocation: MAX_ORDER.
> 
> This is just to silence the alloc_pages warning, right? If so
> how about adding __GFP_NOWARN instead?
> 

The goal was to be more explicit about that, but setting __GFP_NOWARN 
would result in the same functional behavior.  If we're to go that route, 
it would likely be best to add a comment about the limitation.

That said, if AMD would prefer this to be an EINVAL instead of a ENOMEM by 
introducing a more formal limitation on the length that can be used, that 
would be preferred so that we don't need to rely on the page allocator's 
max length to enforce this arbitrarily.

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

* Re: [PATCH] crypto: ccp - Limit memory allocation in SEV_GET_ID2 ioctl
  2022-12-28  1:42   ` David Rientjes
@ 2022-12-28  8:49     ` Herbert Xu
  2022-12-30 22:01       ` David Rientjes
  0 siblings, 1 reply; 12+ messages in thread
From: Herbert Xu @ 2022-12-28  8:49 UTC (permalink / raw)
  To: David Rientjes
  Cc: Peter Gonda, Andy Nguyen, stable, linux-kernel, linux-crypto,
	John Allen, Thomas . Lendacky

On Tue, Dec 27, 2022 at 05:42:31PM -0800, David Rientjes wrote:
>
> The goal was to be more explicit about that, but setting __GFP_NOWARN 
> would result in the same functional behavior.  If we're to go that route, 
> it would likely be best to add a comment about the limitation.
> 
> That said, if AMD would prefer this to be an EINVAL instead of a ENOMEM by 
> introducing a more formal limitation on the length that can be used, that 
> would be preferred so that we don't need to rely on the page allocator's 
> max length to enforce this arbitrarily.

Ideally the limit should be set according to the object that
you're trying to allocate.  But if that is truly unlimited,
and you don't want to see a warning, then GFP_NOWARN seems to
fit the bill.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

* Re: [PATCH] crypto: ccp - Limit memory allocation in SEV_GET_ID2 ioctl
  2022-12-28  8:49     ` Herbert Xu
@ 2022-12-30 22:01       ` David Rientjes
  2022-12-30 22:18         ` [patch] crypto: ccp - Avoid page allocation failure warning for SEV_GET_ID2 David Rientjes
  0 siblings, 1 reply; 12+ messages in thread
From: David Rientjes @ 2022-12-30 22:01 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Peter Gonda, Andy Nguyen, stable, linux-kernel, linux-crypto,
	John Allen, Thomas . Lendacky

On Wed, 28 Dec 2022, Herbert Xu wrote:

> On Tue, Dec 27, 2022 at 05:42:31PM -0800, David Rientjes wrote:
> >
> > The goal was to be more explicit about that, but setting __GFP_NOWARN 
> > would result in the same functional behavior.  If we're to go that route, 
> > it would likely be best to add a comment about the limitation.
> > 
> > That said, if AMD would prefer this to be an EINVAL instead of a ENOMEM by 
> > introducing a more formal limitation on the length that can be used, that 
> > would be preferred so that we don't need to rely on the page allocator's 
> > max length to enforce this arbitrarily.
> 
> Ideally the limit should be set according to the object that
> you're trying to allocate.  But if that is truly unlimited,
> and you don't want to see a warning, then GFP_NOWARN seems to
> fit the bill.
> 

AMD would be able to speak authoritatively on it, but I think the length 
of the ID isn't to be assumed by software because it will likely change 
later.

I don't think there's an active vulnerability with the currnet code so we 
can likely drop stable@vger.kernel.org for this.  The kzalloc() will fail 
if you try to allocate over 2MB.  If you try to allocate >32KB, the page 
allocator will attempt to reclaim but won't oom kill.  If you try to 
allocate <=32KB, there's the potential for oom kill if nothing is 
reclaimable, but if memory is that scarce I think we have bigger problems.

So __GFP_NOWARN will work, but I also think it's subtle enough that it 
warrants being coupled with a comment.

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

* [patch] crypto: ccp - Avoid page allocation failure warning for SEV_GET_ID2
  2022-12-30 22:01       ` David Rientjes
@ 2022-12-30 22:18         ` David Rientjes
  2023-01-03 14:39           ` Tom Lendacky
  2023-01-06 15:18           ` Herbert Xu
  0 siblings, 2 replies; 12+ messages in thread
From: David Rientjes @ 2022-12-30 22:18 UTC (permalink / raw)
  To: Herbert Xu, David S. Miller
  Cc: Peter Gonda, Andy Nguyen, linux-kernel, linux-crypto, John Allen,
	Tom Lendacky

For SEV_GET_ID2, the user provided length does not have a specified 
limitation because the length of the ID may change in the future.  The 
kernel memory allocation, however, is implicitly limited to 4MB on x86 by 
the page allocator, otherwise the kzalloc() will fail.

When this happens, it is best not to spam the kernel log with the warning.  
Simply fail the allocation and return ENOMEM to the user.

Fixes: d6112ea0cb34 ("crypto: ccp - introduce SEV_GET_ID2 command")
Reported-by: Andy Nguyen <theflow@google.com>
Reported-by: Peter Gonda <pgonda@google.com>
Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 drivers/crypto/ccp/sev-dev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -881,7 +881,14 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
 	input_address = (void __user *)input.address;
 
 	if (input.address && input.length) {
-		id_blob = kzalloc(input.length, GFP_KERNEL);
+		/*
+		 * The length of the ID shouldn't be assumed by software since
+		 * it may change in the future.  The allocation size is limited
+		 * to 1 << (PAGE_SHIFT + MAX_ORDER - 1) by the page allocator.
+		 * If the allocation fails, simply return ENOMEM rather than
+		 * warning in the kernel log.
+		 */
+		id_blob = kzalloc(input.length, GFP_KERNEL | __GFP_NOWARN);
 		if (!id_blob)
 			return -ENOMEM;
 

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

* Re: [patch] crypto: ccp - Avoid page allocation failure warning for SEV_GET_ID2
  2022-12-30 22:18         ` [patch] crypto: ccp - Avoid page allocation failure warning for SEV_GET_ID2 David Rientjes
@ 2023-01-03 14:39           ` Tom Lendacky
  2023-01-03 23:18             ` David Rientjes
  2023-01-06 15:18           ` Herbert Xu
  1 sibling, 1 reply; 12+ messages in thread
From: Tom Lendacky @ 2023-01-03 14:39 UTC (permalink / raw)
  To: David Rientjes, Herbert Xu, David S. Miller
  Cc: Peter Gonda, Andy Nguyen, linux-kernel, linux-crypto, John Allen

On 12/30/22 16:18, David Rientjes wrote:
> For SEV_GET_ID2, the user provided length does not have a specified
> limitation because the length of the ID may change in the future.  The
> kernel memory allocation, however, is implicitly limited to 4MB on x86 by
> the page allocator, otherwise the kzalloc() will fail.
> 
> When this happens, it is best not to spam the kernel log with the warning.
> Simply fail the allocation and return ENOMEM to the user.
> 
> Fixes: d6112ea0cb34 ("crypto: ccp - introduce SEV_GET_ID2 command")
> Reported-by: Andy Nguyen <theflow@google.com>
> Reported-by: Peter Gonda <pgonda@google.com>
> Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>   drivers/crypto/ccp/sev-dev.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -881,7 +881,14 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd *argp)
>   	input_address = (void __user *)input.address;
>   
>   	if (input.address && input.length) {
> -		id_blob = kzalloc(input.length, GFP_KERNEL);
> +		/*
> +		 * The length of the ID shouldn't be assumed by software since
> +		 * it may change in the future.  The allocation size is limited
> +		 * to 1 << (PAGE_SHIFT + MAX_ORDER - 1) by the page allocator.
> +		 * If the allocation fails, simply return ENOMEM rather than
> +		 * warning in the kernel log.
> +		 */
> +		id_blob = kzalloc(input.length, GFP_KERNEL | __GFP_NOWARN);

We could do this or we could have the driver invoke the API with a zero 
length to get the minimum buffer size needed for the call. The driver 
could then perform some validation checks comparing the supplied 
input.length to the returned length. If the driver can proceed, then if 
input.length is exactly 2x the minimum length, then kzalloc the 2 * 
minimum length, otherwise kzalloc the minimum length. This is a bit more 
complicated, though, compared to this fix.

Either way, is fine with me. Thoughts?

Thanks,
Tom

>   		if (!id_blob)
>   			return -ENOMEM;
>   

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

* Re: [patch] crypto: ccp - Avoid page allocation failure warning for SEV_GET_ID2
  2023-01-03 14:39           ` Tom Lendacky
@ 2023-01-03 23:18             ` David Rientjes
  2023-01-04 14:40               ` Tom Lendacky
  0 siblings, 1 reply; 12+ messages in thread
From: David Rientjes @ 2023-01-03 23:18 UTC (permalink / raw)
  To: Tom Lendacky
  Cc: Herbert Xu, David S. Miller, Peter Gonda, Andy Nguyen,
	linux-kernel, linux-crypto, John Allen

On Tue, 3 Jan 2023, Tom Lendacky wrote:

> On 12/30/22 16:18, David Rientjes wrote:
> > For SEV_GET_ID2, the user provided length does not have a specified
> > limitation because the length of the ID may change in the future.  The
> > kernel memory allocation, however, is implicitly limited to 4MB on x86 by
> > the page allocator, otherwise the kzalloc() will fail.
> > 
> > When this happens, it is best not to spam the kernel log with the warning.
> > Simply fail the allocation and return ENOMEM to the user.
> > 
> > Fixes: d6112ea0cb34 ("crypto: ccp - introduce SEV_GET_ID2 command")
> > Reported-by: Andy Nguyen <theflow@google.com>
> > Reported-by: Peter Gonda <pgonda@google.com>
> > Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
> > Signed-off-by: David Rientjes <rientjes@google.com>
> > ---
> >   drivers/crypto/ccp/sev-dev.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -881,7 +881,14 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd
> > *argp)
> >   	input_address = (void __user *)input.address;
> >     	if (input.address && input.length) {
> > -		id_blob = kzalloc(input.length, GFP_KERNEL);
> > +		/*
> > +		 * The length of the ID shouldn't be assumed by software since
> > +		 * it may change in the future.  The allocation size is
> > limited
> > +		 * to 1 << (PAGE_SHIFT + MAX_ORDER - 1) by the page allocator.
> > +		 * If the allocation fails, simply return ENOMEM rather than
> > +		 * warning in the kernel log.
> > +		 */
> > +		id_blob = kzalloc(input.length, GFP_KERNEL | __GFP_NOWARN);
> 
> We could do this or we could have the driver invoke the API with a zero length
> to get the minimum buffer size needed for the call. The driver could then
> perform some validation checks comparing the supplied input.length to the
> returned length. If the driver can proceed, then if input.length is exactly 2x
> the minimum length, then kzalloc the 2 * minimum length, otherwise kzalloc the
> minimum length. This is a bit more complicated, though, compared to this fix.
> 

Thanks Tom.  IIUC, this could be useful to identify situations where 
input.length != min_length and input.length != min_length*2 and, in those 
cases, return EINVAL?  Or are there situations where this is actually a 
valid input.length?

I was assuming that the user was always doing its own SEV_GET_ID2 first to 
determine the length and then use it for input.length, but perhaps that's 
not the case and they are passing a bogus value.

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

* Re: [patch] crypto: ccp - Avoid page allocation failure warning for SEV_GET_ID2
  2023-01-03 23:18             ` David Rientjes
@ 2023-01-04 14:40               ` Tom Lendacky
  2023-01-05  1:49                 ` David Rientjes
  0 siblings, 1 reply; 12+ messages in thread
From: Tom Lendacky @ 2023-01-04 14:40 UTC (permalink / raw)
  To: David Rientjes
  Cc: Herbert Xu, David S. Miller, Peter Gonda, Andy Nguyen,
	linux-kernel, linux-crypto, John Allen

On 1/3/23 17:18, David Rientjes wrote:
> On Tue, 3 Jan 2023, Tom Lendacky wrote:
> 
>> On 12/30/22 16:18, David Rientjes wrote:
>>> For SEV_GET_ID2, the user provided length does not have a specified
>>> limitation because the length of the ID may change in the future.  The
>>> kernel memory allocation, however, is implicitly limited to 4MB on x86 by
>>> the page allocator, otherwise the kzalloc() will fail.
>>>
>>> When this happens, it is best not to spam the kernel log with the warning.
>>> Simply fail the allocation and return ENOMEM to the user.
>>>
>>> Fixes: d6112ea0cb34 ("crypto: ccp - introduce SEV_GET_ID2 command")
>>> Reported-by: Andy Nguyen <theflow@google.com>
>>> Reported-by: Peter Gonda <pgonda@google.com>
>>> Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
>>> Signed-off-by: David Rientjes <rientjes@google.com>
>>> ---
>>>    drivers/crypto/ccp/sev-dev.c | 9 ++++++++-
>>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>> --- a/drivers/crypto/ccp/sev-dev.c
>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>> @@ -881,7 +881,14 @@ static int sev_ioctl_do_get_id2(struct sev_issue_cmd
>>> *argp)
>>>    	input_address = (void __user *)input.address;
>>>      	if (input.address && input.length) {
>>> -		id_blob = kzalloc(input.length, GFP_KERNEL);
>>> +		/*
>>> +		 * The length of the ID shouldn't be assumed by software since
>>> +		 * it may change in the future.  The allocation size is
>>> limited
>>> +		 * to 1 << (PAGE_SHIFT + MAX_ORDER - 1) by the page allocator.
>>> +		 * If the allocation fails, simply return ENOMEM rather than
>>> +		 * warning in the kernel log.
>>> +		 */
>>> +		id_blob = kzalloc(input.length, GFP_KERNEL | __GFP_NOWARN);
>>
>> We could do this or we could have the driver invoke the API with a zero length
>> to get the minimum buffer size needed for the call. The driver could then
>> perform some validation checks comparing the supplied input.length to the
>> returned length. If the driver can proceed, then if input.length is exactly 2x
>> the minimum length, then kzalloc the 2 * minimum length, otherwise kzalloc the
>> minimum length. This is a bit more complicated, though, compared to this fix.
>>
> 
> Thanks Tom.  IIUC, this could be useful to identify situations where
> input.length != min_length and input.length != min_length*2 and, in those
> cases, return EINVAL?  Or are there situations where this is actually a
> valid input.length?
> 
> I was assuming that the user was always doing its own SEV_GET_ID2 first to
> determine the length and then use it for input.length, but perhaps that's
> not the case and they are passing a bogus value.

Except that if the user was always doing that, then we wouldn't be worried 
about this case then. But, I think my method is overkill and the simple 
approach of this patch is the way to go.

Thanks,
Tom


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

* Re: [patch] crypto: ccp - Avoid page allocation failure warning for SEV_GET_ID2
  2023-01-04 14:40               ` Tom Lendacky
@ 2023-01-05  1:49                 ` David Rientjes
  2023-01-05 15:37                   ` Tom Lendacky
  0 siblings, 1 reply; 12+ messages in thread
From: David Rientjes @ 2023-01-05  1:49 UTC (permalink / raw)
  To: Tom Lendacky
  Cc: Herbert Xu, David S. Miller, Peter Gonda, Andy Nguyen,
	linux-kernel, linux-crypto, John Allen

On Wed, 4 Jan 2023, Tom Lendacky wrote:

> > > > For SEV_GET_ID2, the user provided length does not have a specified
> > > > limitation because the length of the ID may change in the future.  The
> > > > kernel memory allocation, however, is implicitly limited to 4MB on x86
> > > > by
> > > > the page allocator, otherwise the kzalloc() will fail.
> > > > 
> > > > When this happens, it is best not to spam the kernel log with the
> > > > warning.
> > > > Simply fail the allocation and return ENOMEM to the user.
> > > > 
> > > > Fixes: d6112ea0cb34 ("crypto: ccp - introduce SEV_GET_ID2 command")
> > > > Reported-by: Andy Nguyen <theflow@google.com>
> > > > Reported-by: Peter Gonda <pgonda@google.com>
> > > > Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
> > > > Signed-off-by: David Rientjes <rientjes@google.com>
> > > > ---
> > > >    drivers/crypto/ccp/sev-dev.c | 9 ++++++++-
> > > >    1 file changed, 8 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > > > --- a/drivers/crypto/ccp/sev-dev.c
> > > > +++ b/drivers/crypto/ccp/sev-dev.c
> > > > @@ -881,7 +881,14 @@ static int sev_ioctl_do_get_id2(struct
> > > > sev_issue_cmd
> > > > *argp)
> > > >    	input_address = (void __user *)input.address;
> > > >      	if (input.address && input.length) {
> > > > -		id_blob = kzalloc(input.length, GFP_KERNEL);
> > > > +		/*
> > > > +		 * The length of the ID shouldn't be assumed by software since
> > > > +		 * it may change in the future.  The allocation size is
> > > > limited
> > > > +		 * to 1 << (PAGE_SHIFT + MAX_ORDER - 1) by the page allocator.
> > > > +		 * If the allocation fails, simply return ENOMEM rather than
> > > > +		 * warning in the kernel log.
> > > > +		 */
> > > > +		id_blob = kzalloc(input.length, GFP_KERNEL | __GFP_NOWARN);
> > > 
> > > We could do this or we could have the driver invoke the API with a zero
> > > length
> > > to get the minimum buffer size needed for the call. The driver could then
> > > perform some validation checks comparing the supplied input.length to the
> > > returned length. If the driver can proceed, then if input.length is
> > > exactly 2x
> > > the minimum length, then kzalloc the 2 * minimum length, otherwise kzalloc
> > > the
> > > minimum length. This is a bit more complicated, though, compared to this
> > > fix.
> > > 
> > 
> > Thanks Tom.  IIUC, this could be useful to identify situations where
> > input.length != min_length and input.length != min_length*2 and, in those
> > cases, return EINVAL?  Or are there situations where this is actually a
> > valid input.length?
> > 
> > I was assuming that the user was always doing its own SEV_GET_ID2 first to
> > determine the length and then use it for input.length, but perhaps that's
> > not the case and they are passing a bogus value.
> 
> Except that if the user was always doing that, then we wouldn't be worried
> about this case then. But, I think my method is overkill and the simple
> approach of this patch is the way to go.
> 

Makes sense, thanks for the clarification.  Does that translate into an 
acked-by? :)

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

* Re: [patch] crypto: ccp - Avoid page allocation failure warning for SEV_GET_ID2
  2023-01-05  1:49                 ` David Rientjes
@ 2023-01-05 15:37                   ` Tom Lendacky
  0 siblings, 0 replies; 12+ messages in thread
From: Tom Lendacky @ 2023-01-05 15:37 UTC (permalink / raw)
  To: David Rientjes
  Cc: Herbert Xu, David S. Miller, Peter Gonda, Andy Nguyen,
	linux-kernel, linux-crypto, John Allen

On 1/4/23 19:49, David Rientjes wrote:
> On Wed, 4 Jan 2023, Tom Lendacky wrote:
> 
>>>>> For SEV_GET_ID2, the user provided length does not have a specified
>>>>> limitation because the length of the ID may change in the future.  The
>>>>> kernel memory allocation, however, is implicitly limited to 4MB on x86
>>>>> by
>>>>> the page allocator, otherwise the kzalloc() will fail.
>>>>>
>>>>> When this happens, it is best not to spam the kernel log with the
>>>>> warning.
>>>>> Simply fail the allocation and return ENOMEM to the user.
>>>>>
>>>>> Fixes: d6112ea0cb34 ("crypto: ccp - introduce SEV_GET_ID2 command")
>>>>> Reported-by: Andy Nguyen <theflow@google.com>
>>>>> Reported-by: Peter Gonda <pgonda@google.com>
>>>>> Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
>>>>> Signed-off-by: David Rientjes <rientjes@google.com>
>>>>> ---
>>>>>     drivers/crypto/ccp/sev-dev.c | 9 ++++++++-
>>>>>     1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>>>> --- a/drivers/crypto/ccp/sev-dev.c
>>>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>>>> @@ -881,7 +881,14 @@ static int sev_ioctl_do_get_id2(struct
>>>>> sev_issue_cmd
>>>>> *argp)
>>>>>     	input_address = (void __user *)input.address;
>>>>>       	if (input.address && input.length) {
>>>>> -		id_blob = kzalloc(input.length, GFP_KERNEL);
>>>>> +		/*
>>>>> +		 * The length of the ID shouldn't be assumed by software since
>>>>> +		 * it may change in the future.  The allocation size is
>>>>> limited
>>>>> +		 * to 1 << (PAGE_SHIFT + MAX_ORDER - 1) by the page allocator.
>>>>> +		 * If the allocation fails, simply return ENOMEM rather than
>>>>> +		 * warning in the kernel log.
>>>>> +		 */
>>>>> +		id_blob = kzalloc(input.length, GFP_KERNEL | __GFP_NOWARN);
>>>>
>>>> We could do this or we could have the driver invoke the API with a zero
>>>> length
>>>> to get the minimum buffer size needed for the call. The driver could then
>>>> perform some validation checks comparing the supplied input.length to the
>>>> returned length. If the driver can proceed, then if input.length is
>>>> exactly 2x
>>>> the minimum length, then kzalloc the 2 * minimum length, otherwise kzalloc
>>>> the
>>>> minimum length. This is a bit more complicated, though, compared to this
>>>> fix.
>>>>
>>>
>>> Thanks Tom.  IIUC, this could be useful to identify situations where
>>> input.length != min_length and input.length != min_length*2 and, in those
>>> cases, return EINVAL?  Or are there situations where this is actually a
>>> valid input.length?
>>>
>>> I was assuming that the user was always doing its own SEV_GET_ID2 first to
>>> determine the length and then use it for input.length, but perhaps that's
>>> not the case and they are passing a bogus value.
>>
>> Except that if the user was always doing that, then we wouldn't be worried
>> about this case then. But, I think my method is overkill and the simple
>> approach of this patch is the way to go.
>>
> 
> Makes sense, thanks for the clarification.  Does that translate into an
> acked-by? :)

Ah, yeah, forgot about that!

Acked-by: Tom Lendacky <thomas.lendacky@amd.com>

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

* Re: [patch] crypto: ccp - Avoid page allocation failure warning for SEV_GET_ID2
  2022-12-30 22:18         ` [patch] crypto: ccp - Avoid page allocation failure warning for SEV_GET_ID2 David Rientjes
  2023-01-03 14:39           ` Tom Lendacky
@ 2023-01-06 15:18           ` Herbert Xu
  1 sibling, 0 replies; 12+ messages in thread
From: Herbert Xu @ 2023-01-06 15:18 UTC (permalink / raw)
  To: David Rientjes
  Cc: David S. Miller, Peter Gonda, Andy Nguyen, linux-kernel,
	linux-crypto, John Allen, Tom Lendacky

On Fri, Dec 30, 2022 at 02:18:46PM -0800, David Rientjes wrote:
> For SEV_GET_ID2, the user provided length does not have a specified 
> limitation because the length of the ID may change in the future.  The 
> kernel memory allocation, however, is implicitly limited to 4MB on x86 by 
> the page allocator, otherwise the kzalloc() will fail.
> 
> When this happens, it is best not to spam the kernel log with the warning.  
> Simply fail the allocation and return ENOMEM to the user.
> 
> Fixes: d6112ea0cb34 ("crypto: ccp - introduce SEV_GET_ID2 command")
> Reported-by: Andy Nguyen <theflow@google.com>
> Reported-by: Peter Gonda <pgonda@google.com>
> Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  drivers/crypto/ccp/sev-dev.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

end of thread, other threads:[~2023-01-06 15:20 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-14 20:20 [PATCH] crypto: ccp - Limit memory allocation in SEV_GET_ID2 ioctl Peter Gonda
2022-12-15 10:05 ` Herbert Xu
2022-12-28  1:42   ` David Rientjes
2022-12-28  8:49     ` Herbert Xu
2022-12-30 22:01       ` David Rientjes
2022-12-30 22:18         ` [patch] crypto: ccp - Avoid page allocation failure warning for SEV_GET_ID2 David Rientjes
2023-01-03 14:39           ` Tom Lendacky
2023-01-03 23:18             ` David Rientjes
2023-01-04 14:40               ` Tom Lendacky
2023-01-05  1:49                 ` David Rientjes
2023-01-05 15:37                   ` Tom Lendacky
2023-01-06 15:18           ` Herbert Xu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.