diff mbox series

crypto: ccp - Limit memory allocation in SEV_GET_ID2 ioctl

Message ID 20221214202046.719598-1-pgonda@google.com
State New
Headers
Series crypto: ccp - Limit memory allocation in SEV_GET_ID2 ioctl |

Commit Message

Peter Gonda Dec. 14, 2022, 8:20 p.m. UTC 
  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(+)

Comments

Herbert Xu Dec. 15, 2022, 10:05 a.m. UTC | #1 
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,
David Rientjes Dec. 28, 2022, 1:42 a.m. UTC | #2 
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.
Herbert Xu Dec. 28, 2022, 8:49 a.m. UTC | #3 
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,
David Rientjes Dec. 30, 2022, 10:01 p.m. UTC | #4 
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.
diff mbox series

Patch

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) {