linux-kernel - Re: [PATCH v3 6/6] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200729222150.GA970863@google.com>
Date:   Wed, 29 Jul 2020 15:21:50 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Benson Leung <bleung@...omium.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Gwendal Grignou <gwendal@...omium.org>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-input@...r.kernel.org, linux-pwm@...r.kernel.org,
        Yu-Hsuan Hsu <yuhsuan@...omium.org>,
        Prashant Malani <pmalani@...omium.org>
Subject: Re: [PATCH v3 6/6] platform/chrome: cros_ec_proto: Convert EC error
 codes to Linux error codes

Hi Guenter,

On Sun, Jul 26, 2020 at 03:01:01PM -0700, Guenter Roeck wrote:
> v3: Use -ENOPROTOOPT for EC_RES_INVALID_VERSION
>     Implement function to convert error codes
> v2: No change
> 
>  drivers/platform/chrome/cros_ec_proto.c | 52 ++++++++++++++++++++-----
>  1 file changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index e5bbec979a2a..a081b8245682 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -15,6 +15,43 @@
>  
>  #define EC_COMMAND_RETRIES	50
>  
> +static const int cros_ec_error_map[] = {
> +	[EC_RES_INVALID_COMMAND] = -EOPNOTSUPP,
> +	[EC_RES_ERROR] = -EIO,
> +	[EC_RES_INVALID_PARAM] = -EINVAL,
> +	[EC_RES_ACCESS_DENIED] = -EACCES,
> +	[EC_RES_INVALID_RESPONSE] = -EPROTO,
> +	[EC_RES_INVALID_VERSION] = -ENOPROTOOPT,
> +	[EC_RES_INVALID_CHECKSUM] = -EBADMSG,
> +	[EC_RES_IN_PROGRESS] = -EINPROGRESS,
> +	[EC_RES_UNAVAILABLE] = -ENODATA,
> +	[EC_RES_TIMEOUT] = -ETIMEDOUT,
> +	[EC_RES_OVERFLOW] = -EOVERFLOW,
> +	[EC_RES_INVALID_HEADER] = -EBADR,
> +	[EC_RES_REQUEST_TRUNCATED] = -EBADR,
> +	[EC_RES_RESPONSE_TOO_BIG] = -EFBIG,
> +	[EC_RES_BUS_ERROR] = -EFAULT,
> +	[EC_RES_BUSY] = -EBUSY,
> +	[EC_RES_INVALID_HEADER_VERSION] = -EBADMSG,
> +	[EC_RES_INVALID_HEADER_CRC] = -EBADMSG,
> +	[EC_RES_INVALID_DATA_CRC] = -EBADMSG,
> +	[EC_RES_DUP_UNAVAILABLE] = -ENODATA,
> +};

Sorry I didn't pay attention to this earlier, but is there any
programmatic way to ensure that we don't have unexpected holes here? If
we do (e.g., we add new error codes, but they aren't contiguous for
whatever reasons), then those will get treated as "success" with your
current patch.

I say "unexpected" hole, because EC_RES_SUCCESS (0) is an expected hole.

> +
> +static int cros_ec_map_error(uint32_t result)
> +{
> +	int ret = 0;
> +
> +	if (result != EC_RES_SUCCESS) {
> +		if (result < ARRAY_SIZE(cros_ec_error_map) && cros_ec_error_map[result])
> +			ret = cros_ec_error_map[result];

^^ Maybe we want to double check 'ret != 0'? Or maybe

			ret = cros_ec_error_map[result];
			if (!ret) {
				ret = -EPROTO;
				dev_err(..., "Unexpected EC result code %d\n", result);
			}

? Could even be WARN_ON(), since this would be an actionable programming
error, not exactly an external factor. Or maybe I'm being paranoid, and
future programmers are perfect.

Otherwise:

Reviewed-by: Brian Norris <briannorris@...omium.org>

> +		else
> +			ret = -EPROTO;
> +	}
> +
> +	return ret;
> +}
> +
>  static int prepare_packet(struct cros_ec_device *ec_dev,
>  			  struct cros_ec_command *msg)
>  {