linux-kernel - Re: [PATCH 1/2] platform/chrome: cros

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] [day] [month] [year] [list]

Message-ID: <CAJ_BA_BvGPfAghiQo8uQSAhdqG3NiWXbO7c-Q_x6cvUUukrzsA@mail.gmail.com>
Date: Mon, 2 Dec 2024 10:16:01 +0100
From: Dawid Niedźwiecki <dawidn@...gle.com>
To: Tzung-Bi Shih <tzungbi@...nel.org>
Cc: Benson Leung <bleung@...omium.org>, chrome-platform@...ts.linux.dev, 
	linux-kernel@...r.kernel.org, chromeos-krk-upstreaming@...gle.com
Subject: Re: [PATCH 1/2] platform/chrome: cros_ec: jump to RW before probing

On Thu, Nov 28, 2024 at 5:29 AM Tzung-Bi Shih <tzungbi@...nel.org> wrote:
>
> On Mon, Nov 25, 2024 at 06:44:45PM +0000, Dawid Niedzwiecki wrote:
> > To avoid such problems, send the RWSIG continue command first, which
> > skips waiting for the jump to RW. Send the command more times, to make
> > sure EC is ready in RW before the start of the actual probing process. If
> > a EC device doesn't support the RWSIG, it will respond with invalid
> > command error code and probing will continue as usual.
>
> I'm wondering should it only send RWSIG_ACTION_CONTINUE if EC_CMD_GET_VERSION
> shows the FW is still in RO.

In theory yes, but in practice it is not necessary. If FW is in RW, it
will return
EC_RES_INVALID_COMMAND (the same case as RWSIG is not supported), so
the job is done.
EC_CMD_GET_VERSION + potentially RWSIG_ACTION_CONTINUE sequence is longer.
Additionally, EC_CMD_GET_VERSION requires bigger buffers to store
versions strings.

>
> Curious about: who (in which use case) is responsible for sending
> RWSIG_ACTION_ABORT if it wants the EC stays in RO?

Currently, it is done by flashrom
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/01195dd492ea196cdc60e467a896369e30e90c5b:src/third_party/flashrom/cros_ec.c;l=480

>
> > diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> [...]
> > @@ -204,6 +204,11 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> >       mutex_init(&ec_dev->lock);
> >       lockdep_set_class(&ec_dev->lock, &ec_dev->lockdep_key);
> >
> > +     /* Send RWSIG continue to jump to RW for devices using RWSIG. */
> > +     err = cros_ec_rwsig_continue(ec_dev);
> > +     if (err)
> > +             dev_warn(dev, "Failed to continue RWSIG: %d\n", err);
>
> Too verbose, use dev_info() instead.

I will change that.

>
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> [...]
> > +int cros_ec_rwsig_continue(struct cros_ec_device *ec_dev)
> > +{
> [...]
> > +     for (int i = 0; i < RWSIG_CONTINUE_RETRIES; i++) {
> > +             ret = cros_ec_send_command(ec_dev, msg);
> > +
> > +             if (ret < 0)
> > +                     error_count++;
>
> Should it just return the error if the transmission fails?

The transmission failure is kind of expected here. It is possible that
FPMCU is not ready, when AP
starts sending commands. That's why the retry (which is removed in
PATCH 2/2) was added in the
cros_ec_get_proto_info function.

>
> > +             else if (msg->result == EC_RES_INVALID_COMMAND)
> > +                     /*
> > +                      * If EC_RES_INVALID_COMMAND is retured, it means RWSIG
> > +                      * is not supported or EC is already in RW, so there is
> > +                      * nothing left to do.
> > +                      */
> > +                     break;
> > +             else if (msg->result != EC_RES_SUCCESS)
> > +                     /* Unexpected command error. */
> > +                     error_count++;
>
> Same as `ret < 0`, should it just return if any unexpected errors?

It should return here. It is unexpected that the command itself
returns error, we should return
in this case. I will change that.

>
> > +             else
> > +                     /*
> > +                      * The EC_CMD_RWSIG_ACTION succeed. Send the command
> > +                      * more times, to make sure EC is in RW. A following
> > +                      * command can timeout, because EC may need some time to
> > +                      * initialize after jump to RW.
> > +                      */
> > +                     error_count = 0;
> > +
> > +             if (error_count >= RWSIG_CONTINUE_MAX_ERRORS_IN_ROW)
> > +                     break;
>
> It could return 0 if `error_count >= RWSIG_CONTINUE_MAX_ERRORS_IN_ROW`.

True. I will fix that.

>
> > +
> > +             if (ret != -ETIMEDOUT)
> > +                     usleep_range(90000, 100000);
> > +     }
> > +
> > +     kfree(msg);
> > +
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(cros_ec_rwsig_continue);