| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20111206061949.GA29152@elte.hu>
Date: Tue, 6 Dec 2011 07:19:49 +0100
From: Ingo Molnar <mingo@...e.hu>
To: Daniel Drake <dsd@...top.org>
Cc: akpm@...ux-foundation.org, mingo@...hat.com, tglx@...utronix.de,
hpa@...or.com, x86@...nel.org, pgf@...top.org,
linux-kernel@...r.kernel.org, dilinger@...ued.net
Subject: Re: [PATCH v2] x86, olpc: add debugfs interface for EC commands
* Daniel Drake <dsd@...top.org> wrote:
> Add a debugfs interface for sending commands to the OLPC Embedded Controller
> (EC) and reading the responses. The EC provides functionality for machine
> identification, battery and AC control, wakeup control, etc.
>
> Having a debugfs interface available is useful for EC development and
> debugging.
>
> Based on code by Paul Fox.
>
> Signed-off-by: Paul Fox <pgf@...top.org>
> Signed-off-by: Daniel Drake <dsd@...top.org>
> ---
> Documentation/ABI/testing/debugfs-olpc | 16 ++++++
> arch/x86/platform/olpc/olpc.c | 86 ++++++++++++++++++++++++++++++++
> 2 files changed, 102 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/ABI/testing/debugfs-olpc
Looks rather useful.
Some bugs and nitpicks i noticed:
> v2: incorporate feedback from Andrew Morton (thanks!): documentation in
> Documentation/, fixed input checking, more correct command bytes construction
>
> diff --git a/Documentation/ABI/testing/debugfs-olpc b/Documentation/ABI/testing/debugfs-olpc
> new file mode 100644
> index 0000000..49b9a4e
> --- /dev/null
> +++ b/Documentation/ABI/testing/debugfs-olpc
> @@ -0,0 +1,16 @@
> +What: /sys/kernel/debug/olpc-ec/generic
> +Date: Dec 2011
> +KernelVersion: 3.3
> +Contact: devel@...ts.laptop.org
> +Description:
> +
> +A generic interface for executing OLPC Embedded Controller commands and
> +reading their responses.
> +
> +To execute a command, write data with the format: CC:N A A A A
> +CC is the (hex) command, N is the count of expected reply bytes, and A A A A
> +are optional (hex) arguments.
> +
> +To read the response (if any), read from the generic node after executing
> +a command. Hex reply bytes will be returned, *whether or not* they came from
> +the immediately previous command.
> diff --git a/arch/x86/platform/olpc/olpc.c b/arch/x86/platform/olpc/olpc.c
> index 7cce722..8260747 100644
> --- a/arch/x86/platform/olpc/olpc.c
> +++ b/arch/x86/platform/olpc/olpc.c
> @@ -20,6 +20,7 @@
> #include <linux/platform_device.h>
> #include <linux/of.h>
> #include <linux/syscore_ops.h>
> +#include <linux/debugfs.h>
>
> #include <asm/geode.h>
> #include <asm/setup.h>
> @@ -31,6 +32,13 @@ EXPORT_SYMBOL_GPL(olpc_platform_info);
>
> static DEFINE_SPINLOCK(ec_lock);
>
> +/* debugfs interface to EC commands */
> +#define EC_MAX_CMD_ARGS (5 + 1) /* cmd byte + 5 args */
> +#define EC_MAX_CMD_REPLY (8)
> +static struct dentry *ec_debugfs_dir;
> +static unsigned char ec_debugfs_resp[EC_MAX_CMD_REPLY];
> +static unsigned int ec_debugfs_resp_bytes;
Please put a newline between blocks of defines and variable
definitions, for increased readability.
> +
> /* EC event mask to be applied during suspend (defining wakeup sources). */
> static u16 ec_wakeup_mask;
>
> @@ -269,6 +277,83 @@ int olpc_ec_sci_query(u16 *sci_value)
> }
> EXPORT_SYMBOL_GPL(olpc_ec_sci_query);
>
> +static ssize_t ec_gen_write(struct file *file, const char __user *buf,
> + size_t size, loff_t *ppos)
> +{
> + int i, m;
> + unsigned char ec_cmd[EC_MAX_CMD_ARGS];
> + unsigned int ec_cmd_int[EC_MAX_CMD_ARGS];
> + char cmdbuf[64];
> + int ec_cmd_bytes;
> +
> + size = simple_write_to_buffer(cmdbuf, sizeof(cmdbuf), ppos, buf, size);
> +
> + m = sscanf(cmdbuf, "%x:%u %x %x %x %x %x", &ec_cmd_int[0],
> + &ec_debugfs_resp_bytes,
> + &ec_cmd_int[1], &ec_cmd_int[2], &ec_cmd_int[3],
> + &ec_cmd_int[4], &ec_cmd_int[5]);
> + if (m < 2 || ec_debugfs_resp_bytes > EC_MAX_CMD_REPLY) {
> + printk(KERN_DEBUG "olpc-ec: bad ec cmd: "
> + "cmd:response-count [arg1 [arg2 ...]]\n");
> + return -EINVAL;
Hm, this looks like a potential overflow. If this sscanf() fails
because of a bug in user-space which sets ec_debugfs_resp_bytes
to say 1000, then despite the -EINVAL the ec_debugfs_res_bytes
lingers and any subsequent read() will over-read and over-write
buffers.
> + }
> +
> + /* convert scanf'd ints to char */
> + ec_cmd_bytes = m - 2;
> + for (i = 0; i <= ec_cmd_bytes; i++)
> + ec_cmd[i] = ec_cmd_int[i];
> +
> + printk(KERN_DEBUG "olpc-ec: debugfs cmd 0x%02x with %d args "
> + "%02x %02x %02x %02x %02x, want %d returns\n",
> + ec_cmd[0], ec_cmd_bytes, ec_cmd[1], ec_cmd[2], ec_cmd[3],
> + ec_cmd[4], ec_cmd[5], ec_debugfs_resp_bytes);
> +
> + olpc_ec_cmd((unsigned char) ec_cmd[0],
> + (ec_cmd_bytes == 0) ? NULL : &ec_cmd[1],
> + ec_cmd_bytes, ec_debugfs_resp, ec_debugfs_resp_bytes);
Why the cast? ec_cmd[] is unsigned char already.
> +static ssize_t ec_gen_read(struct file *file, char __user *buf,
> + size_t size, loff_t *ppos)
> +{
> + unsigned int i, r;
> + char *rp;
> + char respbuf[64];
> +
> + rp = respbuf;
> + rp += sprintf(rp, "%02x", ec_debugfs_resp[0]);
> + for (i = 1; i < ec_debugfs_resp_bytes; i++)
> + rp += sprintf(rp, ", %02x", ec_debugfs_resp[i]);
Is the first byte of the response packet uninteresting?
Also, ec_debugfs_resp_bytes is not checked against the limit of
64 of the on-kernel-stack buffer - see the write() comment
above.
> + rp += sprintf(rp, "\n");
> +
> + r = rp - respbuf;
> +
> + return simple_read_from_buffer(buf, size, ppos, respbuf, r);
> +}
> +
> +static const struct file_operations ec_debugfs_genops = {
> + .write = ec_gen_write,
> + .read = ec_gen_read,
Hm, what protects the ec_debugfs_resp[] buffer against
concurrent read()s/write()s?
I realize that this is just a hack for you to feed olpc_ec_cmd()
and see the results, and that ec_lock protects the hardware
itself, but still - a mutex would seem in order. That would also
protect ec_debugfs_resp_bytes.
> +static void setup_debugfs(void)
> +{
> + ec_debugfs_dir = debugfs_create_dir("olpc-ec", 0);
> + if (ec_debugfs_dir == ERR_PTR(-ENODEV))
> + return;
> +
> + debugfs_create_file("generic", 0600, ec_debugfs_dir, NULL,
> + &ec_debugfs_genops);
> +}
So a debug hack is named 'generic'? Shouldnt this be named
something like "cmds" or such?
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists