Switching to memdup_user() overwrites the allocated memory only once,
whereas kzalloc() followed by copy_from_user() initializes the allocated
memory to zero and then immediately overwrites it.
Fixes the following Coccinelle/coccicheck warning reported by
memdup_user.cocci:
WARNING opportunity for memdup_user
Signed-off-by: Thorsten Blum <[email protected]>
---
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.0
…
> 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
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 <[email protected]>
---
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
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.
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 <[email protected]>
> ---
> 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
Hi Dan,
On 27. May 2024, at 12:38, Dan Carpenter <[email protected]> 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
On Thu, May 30, 2024 at 04:15:51PM +0200, Thorsten Blum wrote:
> Hi Dan,
>
> On 27. May 2024, at 12:38, Dan Carpenter <[email protected]> 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