[PATCH] Platform: OLPC: uninitialized data in debugfs write

[PATCH] Platform: OLPC: uninitialized data in debugfs write

[Dan Carpenter]

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(-)

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);
-- 
2.35.1

Re: [PATCH] Platform: OLPC: uninitialized data in debugfs write

[Dan Carpenter]

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

Re: [PATCH] Platform: OLPC: uninitialized data in debugfs write

[Hans de Goede]

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);