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]

Date:   Wed, 29 Jul 2020 16:29:16 -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 <linux-kernel@...r.kernel.org>,
        linux-input@...r.kernel.org, linux-pwm <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

On Wed, Jul 29, 2020 at 4:22 PM Guenter Roeck <linux@...ck-us.net> wrote:
> On 7/29/20 3:21 PM, Brian Norris wrote:
> > On Sun, Jul 26, 2020 at 03:01:01PM -0700, Guenter Roeck wrote:
> >> --- a/drivers/platform/chrome/cros_ec_proto.c
> >> +++ b/drivers/platform/chrome/cros_ec_proto.c

> > ^^ Maybe we want to double check 'ret != 0'? Or maybe
> >
> >                       ret = cros_ec_error_map[result];
> >                       if (!ret) {
>
> 'ret' won't ever be 0 here. Above:
>                                                         && cros_ec_error_map[result]
>
> and below:
>
>                 else
>                         ret = -EPROTO;

Ah, I'm reading too quickly. You're correct, sorry.

> >                               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.
> >
> I think, if anything, we might consider adding the message below (result >=
> ARRAY_SIZE(cros_ec_error_map) is just as bad). Not sure myself. I am
> open to adding it if people think it would be useful/desirable.

No, my primary motivation was that I thought the logic left room for
error if there were holes. I was mistaken on that point. Secondarily,
it was also potentially useful to point out when we fell into those
holes. I'm not sure logging the warning is that important. Generally,
we only care about a handful of result codes, and as long as the rest
don't end up as "success", I think we're in OK shape.

Sorry for the noise. Here's my tag (which given my misreading so far,
should probably have a heavy discount on its value):

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