| Message ID | 20240527083628.210491-2-thorsten.blum@toblux.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] platform/x86/amd/pmf: Use memdup_user() | expand |
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 <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
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
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
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;
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(-)