Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 4 Jan 2023 17:49:13 -0800 (PST)
From:   David Rientjes <rientjes@google.com>
To:     Tom Lendacky <thomas.lendacky@amd.com>
cc:     Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        Peter Gonda <pgonda@google.com>,
        Andy Nguyen <theflow@google.com>, linux-kernel@vger.kernel.org,
        linux-crypto@vger.kernel.org, John Allen <john.allen@amd.com>
Subject: Re: [patch] crypto: ccp - Avoid page allocation failure warning for
 SEV_GET_ID2
In-Reply-To: <260364a5-f467-f83b-b180-583576ce70ee@amd.com>
Message-ID: <271fc4e5-c4cb-a086-fb7f-8b9389570af4@google.com>
References: <20221214202046.719598-1-pgonda@google.com> <Y5rxd6ZVBqFCBOUT@gondor.apana.org.au> <762d33dc-b5fd-d1ef-848c-7de3a6695557@google.com> <Y6wDJd3hfztLoCp1@gondor.apana.org.au> <826b3dda-5b48-2d42-96b8-c49ccebfdfed@google.com>
 <833b4dd0-7f85-b336-0786-965f3f573f74@google.com> <1000d0c8-bd8c-8958-d54f-7e1924fd433d@amd.com> <06de8454-2b29-f3b6-7cf2-c037c2735b6d@google.com> <260364a5-f467-f83b-b180-583576ce70ee@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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? :)