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