linux-kernel - Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

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: <aGZrhe8Ku7eEIRqm@google.com>
Date: Thu, 3 Jul 2025 11:37:41 +0000
From: Tzung-Bi Shih <tzungbi@...nel.org>
To: Dawid Niedźwiecki <dawidn@...gle.com>
Cc: Benson Leung <bleung@...omium.org>, chrome-platform@...ts.linux.dev,
	linux-kernel@...r.kernel.org, chromeos-krk-upstreaming@...gle.com,
	Łukasz Bartosik <ukaszb@...omium.org>
Subject: Re: [PATCH] platform/chrome: Add ChromeOS EC USB driver

On Wed, Jul 02, 2025 at 09:43:40AM +0200, Dawid Niedźwiecki wrote:
> > They doesn't look like shortcomings to me. The corresponding destructor
> > callbacks have to be called when a device is removed anyway.
> >
> 
> Yes, anything related to USB communication itself has to be freed, but
> freeing the cros_ec_device structure and calling the cros_ec_unregister
> function can crash the system if any of a userland application has the
> original file cros_ec open and tries to send a command. The chardev driver
> is not aware that the device has been removed and will try to access the
> removed structures.

We have seen a similar crash before but I didn't come out with a solution
at that time. Could you please try [3]?

[3]: https://patchwork.kernel.org/project/chrome-platform/cover/20250703113509.2511758-1-tzungbi@kernel.org/

> > Instead, re-using the same inode for the userland interface however
> > *silently* swapping the underlying devices makes less sense to me.
> >
> 
> We are sure it is the same EC device, so why do you think accessing the
> same inode makes less sense? It is a case for specific interface, that reboot
> causes reprobing, which is not a case for other interfaces. I believe
> it should be
> transparent for the cros_ec device user, what interface type is used
> e.g. if it is
> SPI, you can reboot the EC device and wait until it responds to a next command,
> but if it is USB, you would need to reopen the cros_ec file after reboot.

Did some experiments and I understand the difference now. It probably depends
on the bus implementation.

1) SCP over RPMSG
# ectool --name=cros_scp reboot_ec
# dmesg
cros-ec-rpmsg 10500000.scp.cros-ec-rpmsg.13.-1: rpmsg send timeout
mtk-scp 10500000.scp: SCP watchdog timeout! 0x0
remoteproc remoteproc0: crash detected in scp: type watchdog
...
cros-ec-rpmsg 10500000.scp.cros-ec-rpmsg.13.-1: Chrome EC device registered

2) FP over SPI
# ectool --name=cros_fp reboot_ec
# dmesg
cros-ec-spi spi-PRP0001:01: EC failed to respond in time

Anyway, I see your point about the EC over USB gets re-probe once it reboots,
sysjumps, crashes, firmware updates, and etc.

Given that:
- The crash you encountered is a common issue for all cros_ec_X drivers.
- I prefer to keep cros_ec_X drivers simple and similar rather than have
  something special (e.g. the memorized `struct cros_ec_device` in current
  cros_ec_usb) for fixing the crash.
Could you give [3] a try to see if it fixes the crash and also call
cros_ec_register()/cros_ec_unregister() everytime in the probe/disconnect?

> > No, I don't think so. I think all EC-like devices share the same concern
> > regardless of the transport medium (e.g. SCP over RPMSG, ISH over ISHTP).
> 
> Yes, you can run a userland program that opens a cros_ec file and constantly
> sends commands e.g. ectool stress, and then manually unbind the ec
> device, but I believe it can cause some crashes/memory leakage. I don't think
> all drivers are adjusted to hot-plugging.

Yes, we have seen a similar crash before. See comment above.