linux-kernel - Re: [patch] crypto: ccp - Avoid page allocation failure warning for SEV_GET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <260364a5-f467-f83b-b180-583576ce70ee@amd.com>
Date:   Wed, 4 Jan 2023 08:40:06 -0600
From:   Tom Lendacky <thomas.lendacky@....com>
To:     David Rientjes <rientjes@...gle.com>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Peter Gonda <pgonda@...gle.com>,
        Andy Nguyen <theflow@...gle.com>, linux-kernel@...r.kernel.org,
        linux-crypto@...r.kernel.org, John Allen <john.allen@....com>
Subject: Re: [patch] crypto: ccp - Avoid page allocation failure warning for
 SEV_GET_ID2

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@...gle.com>
>>> Reported-by: Peter Gonda <pgonda@...gle.com>
>>> Suggested-by: Herbert Xu <herbert@...dor.apana.org.au>
>>> Signed-off-by: David Rientjes <rientjes@...gle.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