* [PATCH] platform/x86: intel_pmc_core: re-write copy in pmc_core_lpm_latch_mode_write()
@ 2021-04-21 9:34 Dan Carpenter
2021-04-21 14:11 ` Hans de Goede
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2021-04-21 9:34 UTC (permalink / raw)
To: Rajneesh Bhardwaj
Cc: David E Box, Hans de Goede, Mark Gross, David E. Box,
platform-driver-x86, linux-kernel, kernel-janitors
There are two bugs in this code:
1) "ret" is unsigned so the error handling is broken.
2) simple_write_to_buffer() is innappropriate. It will succeed even if
we are only able to copy a single byte of data from user space. This
could lead to an information leak if the buf[] array is not fully
initialized.
I've fixed it to use strncpy_from_user() and to return -EINVAL if the
user supplied string is not NUL terminated.
Fixes: 8074a79fad2e ("platform/x86: intel_pmc_core: Add option to set/clear LPM mode")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/platform/x86/intel_pmc_core.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index 3ae00ac85c75..c989796a5d52 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -1360,18 +1360,19 @@ static ssize_t pmc_core_lpm_latch_mode_write(struct file *file,
struct pmc_dev *pmcdev = s->private;
bool clear = false, c10 = false;
unsigned char buf[8];
- size_t ret;
- int idx, m, mode;
+ int idx, m, mode, ret;
+ size_t len;
u32 reg;
- if (count > sizeof(buf) - 1)
+ if (count > sizeof(buf))
return -EINVAL;
- ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
+ len = min(count, sizeof(buf));
+ ret = strncpy_from_user(buf, userbuf, len);
if (ret < 0)
return ret;
-
- buf[count] = '\0';
+ if (ret == len)
+ return -EINVAL;
/*
* Allowed strings are:
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] platform/x86: intel_pmc_core: re-write copy in pmc_core_lpm_latch_mode_write()
2021-04-21 9:34 [PATCH] platform/x86: intel_pmc_core: re-write copy in pmc_core_lpm_latch_mode_write() Dan Carpenter
@ 2021-04-21 14:11 ` Hans de Goede
2021-04-21 15:19 ` [PATCH v2] platform/x86: intel_pmc_core: Uninitialized data " Dan Carpenter
2021-04-21 15:32 ` [PATCH] platform/x86: intel_pmc_core: re-write copy " Dan Carpenter
0 siblings, 2 replies; 5+ messages in thread
From: Hans de Goede @ 2021-04-21 14:11 UTC (permalink / raw)
To: Dan Carpenter, Rajneesh Bhardwaj
Cc: David E Box, Mark Gross, David E. Box, platform-driver-x86,
linux-kernel, kernel-janitors
Hi Dan,
On 4/21/21 11:34 AM, Dan Carpenter wrote:
> There are two bugs in this code:
> 1) "ret" is unsigned so the error handling is broken.
This is already fixed in the latest for-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next
> 2) simple_write_to_buffer() is innappropriate. It will succeed even if
> we are only able to copy a single byte of data from user space. This
> could lead to an information leak if the buf[] array is not fully
> initialized.
>
> I've fixed it to use strncpy_from_user() and to return -EINVAL if the
> user supplied string is not NUL terminated.
This is a debugfs interface, AFAIK there is no guarantee that:
echo foo > /sys/kernel/debug/foo/bar
Will result in the buf of the write(fd, buf, 4 /* 3 chars + '\n' */)
call actually being 0 terminated ? I know that with sysfs the sysfs
code takes care of 0 termination, but I don't believe that that is the
case in debugfs ?
So it would see that the original code which does not assume 0
termination of the user-input is correct here.
Except that you are right that this could result in processing
whatever was leftover in the buffer, since simple_write_to_buffer()
may write less then count bytes to buf.
This should fix that however, while sticking with simple_write_to_buffer():
diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index d174aeb492e0..ac753e1b2cd4 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -1371,7 +1371,7 @@ static ssize_t pmc_core_lpm_latch_mode_write(struct file *file,
if (ret < 0)
return ret;
- buf[count] = '\0';
+ buf[ret] = '\0';
/*
* Allowed strings are:
I think that that would be a better fix ?
Regards,
Hans
>
> Fixes: 8074a79fad2e ("platform/x86: intel_pmc_core: Add option to set/clear LPM mode")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> drivers/platform/x86/intel_pmc_core.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 3ae00ac85c75..c989796a5d52 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1360,18 +1360,19 @@ static ssize_t pmc_core_lpm_latch_mode_write(struct file *file,
> struct pmc_dev *pmcdev = s->private;
> bool clear = false, c10 = false;
> unsigned char buf[8];
> - size_t ret;
> - int idx, m, mode;
> + int idx, m, mode, ret;
> + size_t len;
> u32 reg;
>
> - if (count > sizeof(buf) - 1)
> + if (count > sizeof(buf))
> return -EINVAL;
Assuming that the buffer passed to a debugfs write is guaranteed to be 0 terminated
then this is not necessary, if we hit this case then the ret == len check below
will trigger?
>
> - ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
> + len = min(count, sizeof(buf));
> + ret = strncpy_from_user(buf, userbuf, len);
> if (ret < 0)
> return ret;
> -
> - buf[count] = '\0';
> + if (ret == len)
> + return -EINVAL;
>
> /*
> * Allowed strings are:
>
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2] platform/x86: intel_pmc_core: Uninitialized data in pmc_core_lpm_latch_mode_write()
2021-04-21 14:11 ` Hans de Goede
@ 2021-04-21 15:19 ` Dan Carpenter
2021-04-21 19:32 ` Hans de Goede
2021-04-21 15:32 ` [PATCH] platform/x86: intel_pmc_core: re-write copy " Dan Carpenter
1 sibling, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2021-04-21 15:19 UTC (permalink / raw)
To: Rajneesh Bhardwaj
Cc: David E Box, Hans de Goede, Mark Gross, platform-driver-x86,
linux-kernel, kernel-janitors
The simple_write_to_buffer() can return success if even a single byte
is copied from user space. In this case it can result in using
uninitalized data if the buf[] array is not fully initialized. Really
we should only succeed if the whole buffer is copied.
Just using copy_from_user() is simpler and more appropriate.
Fixes: 8074a79fad2e ("platform/x86: intel_pmc_core: Add option to set/clear LPM mode")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: The first version of this patch returned -EINVAL if userspace didn't
give us NUL terminated strings. That's not necessarily a good
assumption.
This patch is just simpler as well. No need to introduce the "len"
variable because "count" is capped at the start of the function.
drivers/platform/x86/intel_pmc_core.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index d174aeb492e0..b0e486a6bdfb 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -1360,17 +1360,13 @@ static ssize_t pmc_core_lpm_latch_mode_write(struct file *file,
struct pmc_dev *pmcdev = s->private;
bool clear = false, c10 = false;
unsigned char buf[8];
- ssize_t ret;
int idx, m, mode;
u32 reg;
if (count > sizeof(buf) - 1)
return -EINVAL;
-
- ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
- if (ret < 0)
- return ret;
-
+ if (copy_from_user(buf, userbuf, count))
+ return -EFAULT;
buf[count] = '\0';
/*
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] platform/x86: intel_pmc_core: Uninitialized data in pmc_core_lpm_latch_mode_write()
2021-04-21 15:19 ` [PATCH v2] platform/x86: intel_pmc_core: Uninitialized data " Dan Carpenter
@ 2021-04-21 19:32 ` Hans de Goede
0 siblings, 0 replies; 5+ messages in thread
From: Hans de Goede @ 2021-04-21 19:32 UTC (permalink / raw)
To: Dan Carpenter, Rajneesh Bhardwaj
Cc: David E Box, Mark Gross, platform-driver-x86, linux-kernel,
kernel-janitors
Hi,
On 4/21/21 5:19 PM, Dan Carpenter wrote:
> The simple_write_to_buffer() can return success if even a single byte
> is copied from user space. In this case it can result in using
> uninitalized data if the buf[] array is not fully initialized. Really
> we should only succeed if the whole buffer is copied.
>
> Just using copy_from_user() is simpler and more appropriate.
>
> Fixes: 8074a79fad2e ("platform/x86: intel_pmc_core: Add option to set/clear LPM mode")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: The first version of this patch returned -EINVAL if userspace didn't
> give us NUL terminated strings. That's not necessarily a good
> assumption.
>
> This patch is just simpler as well. No need to introduce the "len"
> variable because "count" is capped at the start of the function.
Much better, thank you.
Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.
Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.
Regards,
Hans
> drivers/platform/x86/intel_pmc_core.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index d174aeb492e0..b0e486a6bdfb 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1360,17 +1360,13 @@ static ssize_t pmc_core_lpm_latch_mode_write(struct file *file,
> struct pmc_dev *pmcdev = s->private;
> bool clear = false, c10 = false;
> unsigned char buf[8];
> - ssize_t ret;
> int idx, m, mode;
> u32 reg;
>
> if (count > sizeof(buf) - 1)
> return -EINVAL;
> -
> - ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
> - if (ret < 0)
> - return ret;
> -
> + if (copy_from_user(buf, userbuf, count))
> + return -EFAULT;
> buf[count] = '\0';
>
> /*
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] platform/x86: intel_pmc_core: re-write copy in pmc_core_lpm_latch_mode_write()
2021-04-21 14:11 ` Hans de Goede
2021-04-21 15:19 ` [PATCH v2] platform/x86: intel_pmc_core: Uninitialized data " Dan Carpenter
@ 2021-04-21 15:32 ` Dan Carpenter
1 sibling, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-04-21 15:32 UTC (permalink / raw)
To: Hans de Goede
Cc: Rajneesh Bhardwaj, David E Box, Mark Gross, David E. Box,
platform-driver-x86, linux-kernel, kernel-janitors
On Wed, Apr 21, 2021 at 04:11:15PM +0200, Hans de Goede wrote:
> This should fix that however, while sticking with simple_write_to_buffer():
I really want to get rid of the simple_write_to_buffer(). Using it
would make sense if we wanted the user to be able to seek to the middle
of buf[] but that would just be an info leak.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-04-21 19:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-21 9:34 [PATCH] platform/x86: intel_pmc_core: re-write copy in pmc_core_lpm_latch_mode_write() Dan Carpenter
2021-04-21 14:11 ` Hans de Goede
2021-04-21 15:19 ` [PATCH v2] platform/x86: intel_pmc_core: Uninitialized data " Dan Carpenter
2021-04-21 19:32 ` Hans de Goede
2021-04-21 15:32 ` [PATCH] platform/x86: intel_pmc_core: re-write copy " Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).