Re: [PATCH] platform/x86/amd/pmf: Use memdup_user()

Re: [PATCH] platform/x86/amd/pmf: Use memdup_user()

[Markus Elfring]

…
> Fixes the following Coccinelle/coccicheck warning …

Would a corresponding imperative wording be more desirable for such a change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc7#n94

Regards,
Markus

[PATCH v2] platform/x86/amd/pmf: Use memdup_user()

[Thorsten Blum]

Switch to memdup_user() to overwrite the allocated memory only once
instead of initializing the allocated memory to zero with kzalloc() and
then immediately overwriting it with copy_from_user().

Fix the following Coccinelle/coccicheck warning reported by
memdup_user.cocci:

	WARNING opportunity for memdup_user

Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
Changes in v2:
- Update patch description after feedback from Markus Elfring
---
 drivers/platform/x86/amd/pmf/tee-if.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index b438de4d6bfc..1b53cabc9aa2 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -301,14 +301,9 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
 		return -EINVAL;
 
 	/* re-alloc to the new buffer length of the policy binary */
-	new_policy_buf = kzalloc(length, GFP_KERNEL);
-	if (!new_policy_buf)
-		return -ENOMEM;
-
-	if (copy_from_user(new_policy_buf, buf, length)) {
-		kfree(new_policy_buf);
-		return -EFAULT;
-	}
+	new_policy_buf = memdup_user(buf, length);
+	if (IS_ERR(new_policy_buf))
+		return PTR_ERR(new_policy_buf);
 
 	kfree(dev->policy_buf);
 	dev->policy_buf = new_policy_buf;
-- 
2.45.1

Re: [PATCH v2] platform/x86/amd/pmf: Use memdup_user()

[Ilpo Järvinen]

On Mon, 27 May 2024 10:36:29 +0200, Thorsten Blum wrote:

> Switch to memdup_user() to overwrite the allocated memory only once
> instead of initializing the allocated memory to zero with kzalloc() and
> then immediately overwriting it with copy_from_user().
> 
> Fix the following Coccinelle/coccicheck warning reported by
> memdup_user.cocci:
> 
> [...]


Thank you for your contribution, it has been applied to my local
review-ilpo branch. Note it will show up in the public
platform-drivers-x86/review-ilpo branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/1] platform/x86/amd/pmf: Use memdup_user()
      commit: 46de513068f956b76d68d241a7ad6bc5576d2948

--
 i.

Re: [PATCH v2] platform/x86/amd/pmf: Use memdup_user()

[Dan Carpenter]

On Mon, May 27, 2024 at 10:36:29AM +0200, Thorsten Blum wrote:
> Switch to memdup_user() to overwrite the allocated memory only once
> instead of initializing the allocated memory to zero with kzalloc() and
> then immediately overwriting it with copy_from_user().
> 
> Fix the following Coccinelle/coccicheck warning reported by
> memdup_user.cocci:
> 
> 	WARNING opportunity for memdup_user
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> ---
> Changes in v2:
> - Update patch description after feedback from Markus Elfring

Markus always CC's kernel-janitors even though I have asked him not to.
:(

> ---
>  drivers/platform/x86/amd/pmf/tee-if.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index b438de4d6bfc..1b53cabc9aa2 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -301,14 +301,9 @@ static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
>  		return -EINVAL;

This -EINVAL check could be made stricter.  Instead of checking for
zero it could check for the limit from amd_pmf_start_policy_engine():

	if (dev->policy_sz < POLICY_COOKIE_OFFSET + sizeof(*header))
		return -EINVAL;

Also this check isn't great:

	if (dev->policy_sz < header->length + 512)

header->length is a u32 that comes from the user, so the addition can
overflow.  I can't immediately see how to exploit this though since we
don't seem to use header->length after this (by itself).

regards,
dan carpenter

Re: [PATCH v2] platform/x86/amd/pmf: Use memdup_user()

[Thorsten Blum]

Hi Dan,

On 27. May 2024, at 12:38, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> Also this check isn't great:
> 
> if (dev->policy_sz < header->length + 512)
> 
> header->length is a u32 that comes from the user, so the addition can
> overflow.  I can't immediately see how to exploit this though since we
> don't seem to use header->length after this (by itself).

How about

	if (header->length > U32_MAX - 512 || dev->policy_sz < header->length + 512)
		return -EINVAL;

to prevent a possible overflow?

header->length is used in the next line

	dev->policy_sz = header->length + 512;

and if the addition overflows, we end up setting dev->policy_sz to an 
invalid value.

Thanks,
Thorsten

Re: [PATCH v2] platform/x86/amd/pmf: Use memdup_user()

[Dan Carpenter]

On Thu, May 30, 2024 at 04:15:51PM +0200, Thorsten Blum wrote:
> Hi Dan,
> 
> On 27. May 2024, at 12:38, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > Also this check isn't great:
> > 
> > if (dev->policy_sz < header->length + 512)
> > 
> > header->length is a u32 that comes from the user, so the addition can
> > overflow.  I can't immediately see how to exploit this though since we
> > don't seem to use header->length after this (by itself).
> 
> How about
> 
> 	if (header->length > U32_MAX - 512 || dev->policy_sz < header->length + 512)
> 		return -EINVAL;
> 
> to prevent a possible overflow?

I've been thinking about this and actually we could do something simpler:

	if (dev->policy_sz < size_add(header->length, 512)) {

> 
> header->length is used in the next line
> 
> 	dev->policy_sz = header->length + 512;

Yeah, but it's not used by itself.  The "header->length + 512" has been
verified as a valid value whether it overflows or not.  Only
"header->length" is wrong.

> 
> and if the addition overflows, we end up setting dev->policy_sz to an 
> invalid value.

regards,
dan carpenter