| 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
| ||
|
Date: Wed, 13 May 2015 12:10:34 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Javier Martinez Canillas <javier.martinez@...labora.co.uk>
Cc: Samuel Ortiz <sameo@...ux.intel.com>,
Olof Johansson <olof@...om.net>,
Doug Anderson <dianders@...omium.org>,
Bill Richardson <wfrichar@...omium.org>,
Simon Glass <sjg@...gle.com>,
Gwendal Grignou <gwendal@...gle.com>,
Stephen Barber <smbarber@...omium.org>,
Filipe Brandenburger <filbranden@...gle.com>,
Todd Broch <tbroch@...omium.org>,
Alexandru M Stan <amstan@...omium.org>,
Heiko Stuebner <heiko@...ech.de>,
linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 04/10] mfd: cros_ec: Use a zero-length array for
command data
On Sat, 09 May 2015, Javier Martinez Canillas wrote:
> Commit 1b84f2a4cd4a ("mfd: cros_ec: Use fixed size arrays to transfer
> data with the EC") modified the struct cros_ec_command fields to not
> use pointers for the input and output buffers and use fixed length
> arrays instead.
>
> This change was made because the cros_ec ioctl API uses that struct
> cros_ec_command to allow user-space to send commands to the EC and
> to get data from the EC. So using pointers made the API not 64-bit
> safe. Unfortunately this approach was not flexible enough for all
> the use-cases since there may be a need to send larger commands
> on newer versions of the EC command protocol.
>
> So to avoid to choose a constant length that it may be too big for
> most commands and thus wasting memory and CPU cycles on copy from
> and to user-space or having a size that is too small for some big
> commands, use a zero-length array that is both 64-bit safe and
> flexible. The same buffer is used for both output and input data
> so the maximum of these values should be used to allocate it.
>
> Suggested-by: Gwendal Grignou <gwendal@...omium.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@...labora.co.uk>
> Tested-by: Heiko Stuebner <heiko@...ech.de>
> ---
>
> Changes since v1:
> - Add Heiko Stuebner Tested-by tag
> - Removed a new blank line at EOF warning. Reported by Heiko Stuebner
> - Use kmalloc instead of kzalloc when the message is later initialized
> Suggested by Gwendal Grignou
> - Pre-allocate struct cros_ec_command instead of dynamically allocate it
> whenever is possible. Suggested by Gwendal Grignou
> - Pre-allocate buffers for the usual cases and only allocate dynamically
> in the heap for bigger sizes. Suggested by Gwendal Grignou
> - Don't access the cros_ec_command received from user-space before doing
> a copy_from_user. Suggested by Gwendal Grignou
> - Only copy from user-space outsize bytes and copy_to_user insize bytes
> Suggested by Gwendal Grignou
> - ec_device_ioctl_xcmd() must return the numbers of bytes read and not 0
> on success. Suggested by Gwendal Grignou
> - Rename alloc_cmd_msg to alloc_lightbar_cmd_msg. Suggested by Gwendal Grignou
> ---
> drivers/i2c/busses/i2c-cros-ec-tunnel.c | 59 ++++++++---
> drivers/input/keyboard/cros_ec_keyb.c | 19 ++--
> drivers/mfd/cros_ec.c | 18 ++--
> drivers/mfd/cros_ec_i2c.c | 4 +-
> drivers/mfd/cros_ec_spi.c | 2 +-
> drivers/platform/chrome/cros_ec_dev.c | 66 +++++++++----
> drivers/platform/chrome/cros_ec_lightbar.c | 152 +++++++++++++++++++----------
> drivers/platform/chrome/cros_ec_lpc.c | 8 +-
> drivers/platform/chrome/cros_ec_sysfs.c | 92 +++++++++--------
> include/linux/mfd/cros_ec.h | 6 +-
> 10 files changed, 273 insertions(+), 153 deletions(-)
[...]
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index 1574a9352a6d..ee8aa8142932 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -41,7 +41,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
> out[2] = msg->outsize;
> csum = out[0] + out[1] + out[2];
> for (i = 0; i < msg->outsize; i++)
> - csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->outdata[i];
> + csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->data[i];
> out[EC_MSG_TX_HEADER_BYTES + msg->outsize] = (uint8_t)(csum & 0xff);
>
> return EC_MSG_TX_PROTO_BYTES + msg->outsize;
> @@ -75,11 +75,13 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> ret = ec_dev->cmd_xfer(ec_dev, msg);
> if (msg->result == EC_RES_IN_PROGRESS) {
> int i;
> - struct cros_ec_command status_msg = { };
> + struct cros_ec_command *status_msg;
> struct ec_response_get_comms_status *status;
> + u8 buf[sizeof(*status_msg) + sizeof(*status)] = { };
This sort of thing is usually frowned upon. Can you allocate and free
buf's memory using the normal kernel helpers please?
[...]
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 14cf522123dd..7eee38abd02a 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -42,8 +42,7 @@ enum {
> * @outsize: Outgoing length in bytes
> * @insize: Max number of bytes to accept from EC
> * @result: EC's response to the command (separate from communication failure)
> - * @outdata: Outgoing data to EC
> - * @indata: Where to put the incoming data from EC
> + * @data: Where to put the incoming data from EC and outgoing data to EC
> */
> struct cros_ec_command {
> uint32_t version;
> @@ -51,8 +50,7 @@ struct cros_ec_command {
> uint32_t outsize;
> uint32_t insize;
> uint32_t result;
> - uint8_t outdata[EC_PROTO2_MAX_PARAM_SIZE];
> - uint8_t indata[EC_PROTO2_MAX_PARAM_SIZE];
> + uint8_t data[0];
uint8_t *data?
> };
>
> /**
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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