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

Hans de Goede
2021-04-21T14:11:24Z

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