Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190805202214.3408-1-ncrews@chromium.org>
In-Reply-To: <20190805202214.3408-1-ncrews@chromium.org>
From:   Nick Crews <ncrews@chromium.org>
Date:   Thu, 22 Aug 2019 13:43:14 -0600
Message-ID: <CAHX4x86jrJAUqCGD51AY65B=Sp0fnwsbgs9Erbyg4zPK_jhFMg@mail.gmail.com>
Subject: Re: [PATCH] platform/chrome: wilco_ec: Add batt_ppid_info command to
 telemetry driver
To:     Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        Benson Leung <bleung@chromium.org>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        Duncan Laurie <dlaurie@chromium.org>,
        Daniel Kurtz <djkurtz@chromium.org>,
        Dmitry Torokhov <dtor@google.com>,
        Simon Glass <sjg@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Friendly bump on this :)

On Mon, Aug 5, 2019 at 2:22 PM Nick Crews <ncrews@chromium.org> wrote:
>
> Add the GET_BATT_PPID_INFO=0x8A command to the allowlist of accepted
> telemetry commands. In addition, since this new command requires
> verifying the contents of some of the arguments, I also restructure
> the request to use a union of the argument structs. Also, zero out the
> request buffer before each request, and change "whitelist" to
> "allowlist".
>
> Signed-off-by: Nick Crews <ncrews@chromium.org>
> ---
>  drivers/platform/chrome/wilco_ec/telemetry.c | 64 +++++++++++++-------
>  1 file changed, 43 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/platform/chrome/wilco_ec/telemetry.c b/drivers/platform/chrome/wilco_ec/telemetry.c
> index 94cdc166c840..b9d03c33d8dc 100644
> --- a/drivers/platform/chrome/wilco_ec/telemetry.c
> +++ b/drivers/platform/chrome/wilco_ec/telemetry.c
> @@ -9,7 +9,7 @@
>   * the OS sends a command to the EC via a write() to a char device,
>   * and can read the response with a read(). The write() request is
>   * verified by the driver to ensure that it is performing only one
> - * of the whitelisted commands, and that no extraneous data is
> + * of the allowlisted commands, and that no extraneous data is
>   * being transmitted to the EC. The response is passed directly
>   * back to the reader with no modification.
>   *
> @@ -59,21 +59,10 @@ static DEFINE_IDA(telem_ida);
>  #define WILCO_EC_TELEM_GET_TEMP_INFO           0x95
>  #define WILCO_EC_TELEM_GET_TEMP_READ           0x2C
>  #define WILCO_EC_TELEM_GET_BATT_EXT_INFO       0x07
> +#define WILCO_EC_TELEM_GET_BATT_PPID_INFO      0x8A
>
>  #define TELEM_ARGS_SIZE_MAX    30
>
> -/**
> - * struct wilco_ec_telem_request - Telemetry command and arguments sent to EC.
> - * @command: One of WILCO_EC_TELEM_GET_* command codes.
> - * @reserved: Must be 0.
> - * @args: The first N bytes are one of telem_args_get_* structs, the rest is 0.
> - */
> -struct wilco_ec_telem_request {
> -       u8 command;
> -       u8 reserved;
> -       u8 args[TELEM_ARGS_SIZE_MAX];
> -} __packed;
> -
>  /*
>   * The following telem_args_get_* structs are embedded within the |args| field
>   * of wilco_ec_telem_request.
> @@ -122,6 +111,32 @@ struct telem_args_get_batt_ext_info {
>         u8 var_args[5];
>  } __packed;
>
> +struct telem_args_get_batt_ppid_info {
> +       u8 always1; /* Should always be 1 */
> +} __packed;
> +
> +/**
> + * struct wilco_ec_telem_request - Telemetry command and arguments sent to EC.
> + * @command: One of WILCO_EC_TELEM_GET_* command codes.
> + * @reserved: Must be 0.
> + * @args: The first N bytes are one of telem_args_get_* structs, the rest is 0.
> + */
> +struct wilco_ec_telem_request {
> +       u8 command;
> +       u8 reserved;
> +       union {
> +               u8 buf[TELEM_ARGS_SIZE_MAX];
> +               struct telem_args_get_log               get_log;
> +               struct telem_args_get_version           get_version;
> +               struct telem_args_get_fan_info          get_fan_info;
> +               struct telem_args_get_diag_info         get_diag_info;
> +               struct telem_args_get_temp_info         get_temp_info;
> +               struct telem_args_get_temp_read         get_temp_read;
> +               struct telem_args_get_batt_ext_info     get_batt_ext_info;
> +               struct telem_args_get_batt_ppid_info    get_batt_ppid_info;
> +       } args;
> +} __packed;
> +
>  /**
>   * check_telem_request() - Ensure that a request from userspace is valid.
>   * @rq: Request buffer copied from userspace.
> @@ -133,7 +148,7 @@ struct telem_args_get_batt_ext_info {
>   * We do not want to allow userspace to send arbitrary telemetry commands to
>   * the EC. Therefore we check to ensure that
>   * 1. The request follows the format of struct wilco_ec_telem_request.
> - * 2. The supplied command code is one of the whitelisted commands.
> + * 2. The supplied command code is one of the allowlisted commands.
>   * 3. The request only contains the necessary data for the header and arguments.
>   */
>  static int check_telem_request(struct wilco_ec_telem_request *rq,
> @@ -146,25 +161,31 @@ static int check_telem_request(struct wilco_ec_telem_request *rq,
>
>         switch (rq->command) {
>         case WILCO_EC_TELEM_GET_LOG:
> -               max_size += sizeof(struct telem_args_get_log);
> +               max_size += sizeof(rq->args.get_log);
>                 break;
>         case WILCO_EC_TELEM_GET_VERSION:
> -               max_size += sizeof(struct telem_args_get_version);
> +               max_size += sizeof(rq->args.get_version);
>                 break;
>         case WILCO_EC_TELEM_GET_FAN_INFO:
> -               max_size += sizeof(struct telem_args_get_fan_info);
> +               max_size += sizeof(rq->args.get_fan_info);
>                 break;
>         case WILCO_EC_TELEM_GET_DIAG_INFO:
> -               max_size += sizeof(struct telem_args_get_diag_info);
> +               max_size += sizeof(rq->args.get_diag_info);
>                 break;
>         case WILCO_EC_TELEM_GET_TEMP_INFO:
> -               max_size += sizeof(struct telem_args_get_temp_info);
> +               max_size += sizeof(rq->args.get_temp_info);
>                 break;
>         case WILCO_EC_TELEM_GET_TEMP_READ:
> -               max_size += sizeof(struct telem_args_get_temp_read);
> +               max_size += sizeof(rq->args.get_temp_read);
>                 break;
>         case WILCO_EC_TELEM_GET_BATT_EXT_INFO:
> -               max_size += sizeof(struct telem_args_get_batt_ext_info);
> +               max_size += sizeof(rq->args.get_batt_ext_info);
> +               break;
> +       case WILCO_EC_TELEM_GET_BATT_PPID_INFO:
> +               if (rq->args.get_batt_ppid_info.always1 != 1)
> +                       return -EINVAL;
> +
> +               max_size += sizeof(rq->args.get_batt_ppid_info);
>                 break;
>         default:
>                 return -EINVAL;
> @@ -250,6 +271,7 @@ static ssize_t telem_write(struct file *filp, const char __user *buf,
>
>         if (count > sizeof(sess_data->request))
>                 return -EMSGSIZE;
> +       memset(&sess_data->request, 0, sizeof(sess_data->request));
>         if (copy_from_user(&sess_data->request, buf, count))
>                 return -EFAULT;
>         ret = check_telem_request(&sess_data->request, count);
> --
> 2.20.1
>