| Message ID | 833b4dd0-7f85-b336-0786-965f3f573f74@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: ccp - Avoid page allocation failure warning for SEV_GET_ID2 | expand |
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; >
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.
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
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? :)
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>
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.
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;
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(-)