| Message ID | YthIKn+TfZSZMEcM@kili (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | Platform: OLPC: uninitialized data in debugfs write | expand |
On Wed, Jul 20, 2022 at 09:23:38PM +0300, Dan Carpenter wrote: > The call to: > > size = simple_write_to_buffer(cmdbuf, sizeof(cmdbuf), ppos, buf, size); > > will succeed if at least one byte is written to the "cmdbuf" buffer. > The "*ppos" value controls which byte is written. Another problem is > that this code does not check for errors so it's possible for the entire > buffer to be unintialized. > > Inintialize the struct to zero to prevent reading uninitialized stack > data. > > Debugfs is normally only writable by root so the impact of this bug is > very minimal. > > Fixes: 6cca83d498bd ("Platform: OLPC: move debugfs support from x86 EC driver") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > The ec_dbgfs_cmd_write() function is not great. We could copy the data > outside the lock for example. But that's outside the scope of this > patch. More relevant another improvement would be to replace the simple_write_to_buffer() with a check for "if (*ppos) return 0;" and a copy_from_user(). The simple_write_to_buffer() function is not appropriate here. However I can't test this code, and this is not really core code so I just did the minimum to fix the bug. regards, dan carpenter
Hi, On 7/20/22 20:23, Dan Carpenter wrote: > The call to: > > size = simple_write_to_buffer(cmdbuf, sizeof(cmdbuf), ppos, buf, size); > > will succeed if at least one byte is written to the "cmdbuf" buffer. > The "*ppos" value controls which byte is written. Another problem is > that this code does not check for errors so it's possible for the entire > buffer to be unintialized. > > Inintialize the struct to zero to prevent reading uninitialized stack > data. > > Debugfs is normally only writable by root so the impact of this bug is > very minimal. > > Fixes: 6cca83d498bd ("Platform: OLPC: move debugfs support from x86 EC driver") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> 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 > --- > The ec_dbgfs_cmd_write() function is not great. We could copy the data > outside the lock for example. But that's outside the scope of this > patch. > > drivers/platform/olpc/olpc-ec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c > index 4ff5c3a12991..921520475ff6 100644 > --- a/drivers/platform/olpc/olpc-ec.c > +++ b/drivers/platform/olpc/olpc-ec.c > @@ -264,7 +264,7 @@ static ssize_t ec_dbgfs_cmd_write(struct file *file, const char __user *buf, > int i, m; > unsigned char ec_cmd[EC_MAX_CMD_ARGS]; > unsigned int ec_cmd_int[EC_MAX_CMD_ARGS]; > - char cmdbuf[64]; > + char cmdbuf[64] = ""; > int ec_cmd_bytes; > > mutex_lock(&ec_dbgfs_lock);
diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c index 4ff5c3a12991..921520475ff6 100644 --- a/drivers/platform/olpc/olpc-ec.c +++ b/drivers/platform/olpc/olpc-ec.c @@ -264,7 +264,7 @@ static ssize_t ec_dbgfs_cmd_write(struct file *file, const char __user *buf, int i, m; unsigned char ec_cmd[EC_MAX_CMD_ARGS]; unsigned int ec_cmd_int[EC_MAX_CMD_ARGS]; - char cmdbuf[64]; + char cmdbuf[64] = ""; int ec_cmd_bytes; mutex_lock(&ec_dbgfs_lock);
The call to: size = simple_write_to_buffer(cmdbuf, sizeof(cmdbuf), ppos, buf, size); will succeed if at least one byte is written to the "cmdbuf" buffer. The "*ppos" value controls which byte is written. Another problem is that this code does not check for errors so it's possible for the entire buffer to be unintialized. Inintialize the struct to zero to prevent reading uninitialized stack data. Debugfs is normally only writable by root so the impact of this bug is very minimal. Fixes: 6cca83d498bd ("Platform: OLPC: move debugfs support from x86 EC driver") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- The ec_dbgfs_cmd_write() function is not great. We could copy the data outside the lock for example. But that's outside the scope of this patch. drivers/platform/olpc/olpc-ec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)