[PATCH] platform/x86: intel_pmc_core: re-write copy in pmc_core_lpm_latch_mode_write()

[PATCH] platform/x86: intel_pmc_core: re-write copy in pmc_core_lpm_latch_mode_write()

[Dan Carpenter]

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

Re: [PATCH] platform/x86: intel_pmc_core: re-write copy in pmc_core_lpm_latch_mode_write()

[Hans de Goede]

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:
> 

[PATCH v2] platform/x86: intel_pmc_core: Uninitialized data in pmc_core_lpm_latch_mode_write()

[Dan Carpenter]

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

Re: [PATCH v2] platform/x86: intel_pmc_core: Uninitialized data in pmc_core_lpm_latch_mode_write()

[Hans de Goede]

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';
>  
>  	/*
> 

Re: [PATCH] platform/x86: intel_pmc_core: re-write copy in pmc_core_lpm_latch_mode_write()

[Dan Carpenter]

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