| Message ID | YH/xicL9RXjH2pvD@mwanda (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: intel_pmc_core: re-write copy in pmc_core_lpm_latch_mode_write() | expand |
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: >
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
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:
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(-)