| 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: Fri, 18 Mar 2022 11:00:19 -0700
From: Daisuke Nojiri <dnojiri@...gle.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Benson Leung <bleung@...omium.org>,
chrome-platform@...ts.linux.dev,
linux-kernel <linux-kernel@...r.kernel.org>,
Daisuke Nojiri <dnojiri@...omium.org>,
Rob Barnes <robbarnes@...gle.com>,
Rajat Jain <rajatja@...gle.com>,
Brian Norris <briannorris@...omium.org>,
Parth Malkan <parthmalkan@...gle.com>
Subject: Re: [PATCH v2] platform/chrome: Re-introduce cros_ec_cmd_xfer and use
it for ioctls
The comment looks good to me. The rest also looks good to me (based on
the FROMLIST patch on Gerrit).
Reviewed-by: Daisuke Nojiri <dnojiri@...omium.org>
On Fri, Mar 18, 2022 at 10:50 AM Daisuke Nojiri <dnojiri@...gle.com> wrote:
>
> The comment looks good to me. The rest also looks good to me (based on the FROMLIST patch on Gerrit).
>
> On Fri, Mar 18, 2022 at 9:56 AM Guenter Roeck <linux@...ck-us.net> wrote:
>>
>> Commit 413dda8f2c6f ("platform/chrome: cros_ec_chardev: Use
>> cros_ec_cmd_xfer_status helper") inadvertendly changed the userspace ABI.
>> Previously, cros_ec ioctls would only report errors if the EC communication
>> failed, and otherwise return success and the result of the EC
>> communication. An EC command execution failure was reported in the EC
>> response field. The above mentioned commit changed this behavior, and the
>> ioctl itself would fail. This breaks userspace commands trying to analyze
>> the EC command execution error since the actual EC command response is no
>> longer reported to userspace.
>>
>> Fix the problem by re-introducing the cros_ec_cmd_xfer() helper, and use it
>> to handle ioctl messages.
>>
>> Fixes: 413dda8f2c6f ("platform/chrome: cros_ec_chardev: Use cros_ec_cmd_xfer_status helper")
>> Cc: Daisuke Nojiri <dnojiri@...omium.org>
>> Cc: Rob Barnes <robbarnes@...gle.com>
>> Cc: Rajat Jain <rajatja@...gle.com>
>> Cc: Brian Norris <briannorris@...omium.org>
>> Cc: Parth Malkan <parthmalkan@...gle.com>
>> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
>> ---
>> v2: Updated comments / return value description. No functional change.
>>
>> drivers/platform/chrome/cros_ec_chardev.c | 2 +-
>> drivers/platform/chrome/cros_ec_proto.c | 50 +++++++++++++++++----
>> include/linux/platform_data/cros_ec_proto.h | 3 ++
>> 3 files changed, 45 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
>> index e0bce869c49a..fd33de546aee 100644
>> --- a/drivers/platform/chrome/cros_ec_chardev.c
>> +++ b/drivers/platform/chrome/cros_ec_chardev.c
>> @@ -301,7 +301,7 @@ static long cros_ec_chardev_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
>> }
>>
>> s_cmd->command += ec->cmd_offset;
>> - ret = cros_ec_cmd_xfer_status(ec->ec_dev, s_cmd);
>> + ret = cros_ec_cmd_xfer(ec->ec_dev, s_cmd);
>> /* Only copy data to userland if data was received. */
>> if (ret < 0)
>> goto exit;
>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>> index c4caf2e2de82..ac1419881ff3 100644
>> --- a/drivers/platform/chrome/cros_ec_proto.c
>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>> @@ -560,22 +560,28 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
>> EXPORT_SYMBOL(cros_ec_query_all);
>>
>> /**
>> - * cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC.
>> + * cros_ec_cmd_xfer() - Send a command to the ChromeOS EC.
>> * @ec_dev: EC device.
>> * @msg: Message to write.
>> *
>> - * Call this to send a command to the ChromeOS EC. This should be used instead of calling the EC's
>> - * cmd_xfer() callback directly. It returns success status only if both the command was transmitted
>> - * successfully and the EC replied with success status.
>> + * Call this to send a command to the ChromeOS EC. This should be used instead
>> + * of calling the EC's cmd_xfer() callback directly. This function does not
>> + * convert EC command execution error codes to Linux error codes. Most
>> + * in-kernel users will want to use cros_ec_cmd_xfer_status() instead since
>> + * that function implements the conversion.
>> *
>> * Return:
>> - * >=0 - The number of bytes transferred
>> - * <0 - Linux error code
>> + * >0 - EC command was executed successfully. The return value is the number
>> + * of bytes returned by the EC (excluding the header).
>> + * =0 - EC communication was successful. EC command execution results are
>> + * reported in msg->result. The result will be EC_RES_SUCCESS if the
>> + * command was executed successfully or report an EC command execution
>> + * error.
>> + * <0 - EC communication error. Return value is the Linux error code.
>> */
>> -int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
>> - struct cros_ec_command *msg)
>> +int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev, struct cros_ec_command *msg)
>> {
>> - int ret, mapped;
>> + int ret;
>>
>> mutex_lock(&ec_dev->lock);
>> if (ec_dev->proto_version == EC_PROTO_VERSION_UNKNOWN) {
>> @@ -616,6 +622,32 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
>> ret = send_command(ec_dev, msg);
>> mutex_unlock(&ec_dev->lock);
>>
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(cros_ec_cmd_xfer);
>> +
>> +/**
>> + * cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC.
>> + * @ec_dev: EC device.
>> + * @msg: Message to write.
>> + *
>> + * Call this to send a command to the ChromeOS EC. This should be used instead of calling the EC's
>> + * cmd_xfer() callback directly. It returns success status only if both the command was transmitted
>> + * successfully and the EC replied with success status.
>> + *
>> + * Return:
>> + * >=0 - The number of bytes transferred.
>> + * <0 - Linux error code
>> + */
>> +int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
>> + struct cros_ec_command *msg)
>> +{
>> + int ret, mapped;
>> +
>> + ret = cros_ec_cmd_xfer(ec_dev, msg);
>> + if (ret < 0)
>> + return ret;
>> +
>> mapped = cros_ec_map_error(msg->result);
>> if (mapped) {
>> dev_dbg(ec_dev->dev, "Command result (err: %d [%d])\n",
>> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
>> index df3c78c92ca2..16931569adce 100644
>> --- a/include/linux/platform_data/cros_ec_proto.h
>> +++ b/include/linux/platform_data/cros_ec_proto.h
>> @@ -216,6 +216,9 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
>> int cros_ec_check_result(struct cros_ec_device *ec_dev,
>> struct cros_ec_command *msg);
>>
>> +int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
>> + struct cros_ec_command *msg);
>> +
>> int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
>> struct cros_ec_command *msg);
>>
>> --
>> 2.35.1
>>
Powered by blists - more mailing lists