mbox series

[v1,0/2] usb: typec: ucsi: sysfs mailbox for commands

Message ID 20250206141936.1117222-1-heikki.krogerus@linux.intel.com
Headers show
Series usb: typec: ucsi: sysfs mailbox for commands | expand

Message

Heikki Krogerus Feb. 6, 2025, 2:19 p.m. UTC 
Hi,

UCSI has commands that can be used to configure the platform policy
manager (PPM, which is the EC in most cases) on top of individual
connectors. That kind of commands are very UCSI specific, and because
of that, don't fit very well into any of our existing device classes
that are all designed to represent the connectors in generic fashion.

Nevertheless, the user space needs some way to configure also the
entire PPM with these commands. Exposing the UCSI data structure as a
mailbox file to the user space felt to me as the simplest way to do
that, so that's why this proposal.

This mailbox is of course not limited to those commands only - any
UCSI command can be send to the PPM with it. Łukasz, would this cover
also your debugging and testing needs that you were planning the
netlink for (although, for the ChromeOS UCSI driver only)?

Br,

Heikki Krogerus (2):
  usb: typec: ucsi: Command mailbox interface for the userspace
  tools: usb: UCSI command testing tool

 Documentation/ABI/testing/sysfs-driver-ucsi |  20 ++
 drivers/usb/typec/ucsi/Makefile             |   2 +-
 drivers/usb/typec/ucsi/sysfs.c              | 127 ++++++++++
 drivers/usb/typec/ucsi/ucsi.c               |  31 ++-
 drivers/usb/typec/ucsi/ucsi.h               |   7 +
 tools/usb/.gitignore                        |   1 +
 tools/usb/Build                             |   1 +
 tools/usb/Makefile                          |   8 +-
 tools/usb/ucsi.c                            | 250 ++++++++++++++++++++
 9 files changed, 432 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-ucsi
 create mode 100644 drivers/usb/typec/ucsi/sysfs.c
 create mode 100644 tools/usb/ucsi.c

Comments

Heikki Krogerus Feb. 7, 2025, 1:04 p.m. UTC | #1 
On Thu, Feb 06, 2025 at 03:51:48PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Feb 06, 2025 at 04:19:31PM +0200, Heikki Krogerus wrote:
> > Some of the UCSI commands can be used to configure the
> > entire Platform Policy Manager (PPM) instead of just
> > individual connectors. To allow the user space communicate
> > those commands with the PPM, adding a mailbox interface. The
> > interface is a single attribute file that represents the
> > main "OPM to PPM" UCSI data structure.
> > 
> > The mailbox allows any UCSI command to be sent to the PPM so
> > it should be also useful for validation, testing and
> > debugging purposes.
> 
> As it's for this type of thing, why not put it in debugfs instead?
> 
> > +static ssize_t ucsi_write(struct file *filp, struct kobject *kobj,
> > +			  const struct bin_attribute *attr,
> > +			  char *buf, loff_t off, size_t count)
> > +{
> > +	struct ucsi_sysfs *sysfs = attr->private;
> > +	struct ucsi *ucsi = sysfs->ucsi;
> > +	int ret;
> > +
> > +	u64 *control = (u64 *)&sysfs->mailbox[UCSI_CONTROL];
> > +	u32 *cci = (u32 *)&sysfs->mailbox[UCSI_CCI];
> > +	void *data = &sysfs->mailbox[UCSI_MESSAGE_IN];
> > +
> > +	/* TODO: MESSAGE_OUT. */
> > +	if (off != UCSI_CONTROL || count != sizeof(*control))
> > +		return -EFAULT;
> > +
> > +	mutex_lock(&sysfs->lock);
> > +
> > +	memset(data, 0, UCSI_MAX_DATA_LENGTH(ucsi));
> > +
> > +	/* PPM_RESET has to be handled separately. */
> > +	*control = get_unaligned_le64(buf);
> > +	if (UCSI_COMMAND(*control) == UCSI_PPM_RESET) {
> > +		ret = ucsi_reset_ppm(ucsi, cci);
> > +		goto out_unlock_sysfs;
> > +	}
> > +
> > +	mutex_lock(&ucsi->ppm_lock);
> > +
> > +	ret = ucsi->ops->sync_control(ucsi, *control, cci, NULL, 0);
> > +	if (ret)
> > +		goto out_unlock_ppm;
> > +
> > +	if (UCSI_CCI_LENGTH(*cci) && ucsi->ops->read_message_in(ucsi, data, UCSI_CCI_LENGTH(*cci)))
> > +		dev_err(ucsi->dev, "failed to read MESSAGE_IN\n");
> > +
> > +	ret = ucsi->ops->sync_control(ucsi, UCSI_ACK_CC_CI | UCSI_ACK_COMMAND_COMPLETE,
> > +				      NULL, NULL, 0);
> > +out_unlock_ppm:
> > +	mutex_unlock(&ucsi->ppm_lock);
> > +out_unlock_sysfs:
> > +	mutex_unlock(&sysfs->lock);
> > +
> > +	return ret ?: count;
> > +}
> 
> This worries me, any userspace tool can now do this?  What other "bad"
> things can it to the connection?

Although, there is actually only a limited number of things that you
can do to the connection using UCSI, that is definitely a concern.

The PPM (which is the EC firmware in most cases) is expected to prevent
any harmful or "unauthorized" UCSI commands from being executed, but
I'm not sure there is any guarantees for that at the moment.

> > +int ucsi_sysfs_register(struct ucsi *ucsi)
> > +{
> > +	struct ucsi_sysfs *sysfs;
> > +	int ret;
> > +
> > +	sysfs = kzalloc(struct_size(sysfs, mailbox, UCSI_MAILBOX_SIZE(ucsi)), GFP_KERNEL);
> > +	if (!sysfs)
> > +		return -ENOMEM;
> > +
> > +	sysfs->ucsi = ucsi;
> > +	mutex_init(&sysfs->lock);
> > +	memcpy(sysfs->mailbox, &ucsi->version, sizeof(ucsi->version));
> > +
> > +	sysfs_bin_attr_init(&sysfs->bin_attr);
> > +
> > +	sysfs->bin_attr.attr.name = "ucsi";
> > +	sysfs->bin_attr.attr.mode = 0644;
> > +
> > +	sysfs->bin_attr.size = UCSI_MAILBOX_SIZE(ucsi);
> > +	sysfs->bin_attr.private = sysfs;
> > +	sysfs->bin_attr.read_new = ucsi_read;
> > +	sysfs->bin_attr.write_new = ucsi_write;
> > +
> > +	ret = sysfs_create_bin_file(&ucsi->dev->kobj, &sysfs->bin_attr);
> 
> You raced with userspace and lost, right?  Why are you dynamically
> creating this attribute, can't you just use a static one?

The size of the attribute depends on the UCSI version.

> But again, why not debugfs?  I'd feel a lot more comfortable with that
> instead of sysfs.

I would actually prefer debugfs for this, but this is in any case
not primarily for debugging and validation.

The initial goal was to supply the user space some way to control the
EC's power related policies using UCSI commands such as
SET_POWER_LEVEL and GET_POWER_LEVEL (guys, please correct me if I got
that wrong).

But I'm now again wondering could those power policy tasks be handled
using the UCSI power supplies after all? Venkat, did you look into
that?

thanks,
Dmitry Baryshkov Feb. 7, 2025, 8:15 p.m. UTC | #2 
On Fri, Feb 07, 2025 at 03:04:34PM +0200, Heikki Krogerus wrote:
> On Thu, Feb 06, 2025 at 03:51:48PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Feb 06, 2025 at 04:19:31PM +0200, Heikki Krogerus wrote:
> > > Some of the UCSI commands can be used to configure the
> > > entire Platform Policy Manager (PPM) instead of just
> > > individual connectors. To allow the user space communicate
> > > those commands with the PPM, adding a mailbox interface. The
> > > interface is a single attribute file that represents the
> > > main "OPM to PPM" UCSI data structure.
> > > 
> > > The mailbox allows any UCSI command to be sent to the PPM so
> > > it should be also useful for validation, testing and
> > > debugging purposes.
> > 
> > As it's for this type of thing, why not put it in debugfs instead?
> > 
> > > +static ssize_t ucsi_write(struct file *filp, struct kobject *kobj,
> > > +			  const struct bin_attribute *attr,
> > > +			  char *buf, loff_t off, size_t count)
> > > +{
> > > +	struct ucsi_sysfs *sysfs = attr->private;
> > > +	struct ucsi *ucsi = sysfs->ucsi;
> > > +	int ret;
> > > +
> > > +	u64 *control = (u64 *)&sysfs->mailbox[UCSI_CONTROL];
> > > +	u32 *cci = (u32 *)&sysfs->mailbox[UCSI_CCI];
> > > +	void *data = &sysfs->mailbox[UCSI_MESSAGE_IN];
> > > +
> > > +	/* TODO: MESSAGE_OUT. */
> > > +	if (off != UCSI_CONTROL || count != sizeof(*control))
> > > +		return -EFAULT;
> > > +
> > > +	mutex_lock(&sysfs->lock);
> > > +
> > > +	memset(data, 0, UCSI_MAX_DATA_LENGTH(ucsi));
> > > +
> > > +	/* PPM_RESET has to be handled separately. */
> > > +	*control = get_unaligned_le64(buf);
> > > +	if (UCSI_COMMAND(*control) == UCSI_PPM_RESET) {
> > > +		ret = ucsi_reset_ppm(ucsi, cci);
> > > +		goto out_unlock_sysfs;
> > > +	}
> > > +
> > > +	mutex_lock(&ucsi->ppm_lock);
> > > +
> > > +	ret = ucsi->ops->sync_control(ucsi, *control, cci, NULL, 0);
> > > +	if (ret)
> > > +		goto out_unlock_ppm;
> > > +
> > > +	if (UCSI_CCI_LENGTH(*cci) && ucsi->ops->read_message_in(ucsi, data, UCSI_CCI_LENGTH(*cci)))
> > > +		dev_err(ucsi->dev, "failed to read MESSAGE_IN\n");
> > > +
> > > +	ret = ucsi->ops->sync_control(ucsi, UCSI_ACK_CC_CI | UCSI_ACK_COMMAND_COMPLETE,
> > > +				      NULL, NULL, 0);
> > > +out_unlock_ppm:
> > > +	mutex_unlock(&ucsi->ppm_lock);
> > > +out_unlock_sysfs:
> > > +	mutex_unlock(&sysfs->lock);
> > > +
> > > +	return ret ?: count;
> > > +}
> > 
> > This worries me, any userspace tool can now do this?  What other "bad"
> > things can it to the connection?
> 
> Although, there is actually only a limited number of things that you
> can do to the connection using UCSI, that is definitely a concern.
> 
> The PPM (which is the EC firmware in most cases) is expected to prevent
> any harmful or "unauthorized" UCSI commands from being executed, but
> I'm not sure there is any guarantees for that at the moment.
> 
> > > +int ucsi_sysfs_register(struct ucsi *ucsi)
> > > +{
> > > +	struct ucsi_sysfs *sysfs;
> > > +	int ret;
> > > +
> > > +	sysfs = kzalloc(struct_size(sysfs, mailbox, UCSI_MAILBOX_SIZE(ucsi)), GFP_KERNEL);
> > > +	if (!sysfs)
> > > +		return -ENOMEM;
> > > +
> > > +	sysfs->ucsi = ucsi;
> > > +	mutex_init(&sysfs->lock);
> > > +	memcpy(sysfs->mailbox, &ucsi->version, sizeof(ucsi->version));
> > > +
> > > +	sysfs_bin_attr_init(&sysfs->bin_attr);
> > > +
> > > +	sysfs->bin_attr.attr.name = "ucsi";
> > > +	sysfs->bin_attr.attr.mode = 0644;
> > > +
> > > +	sysfs->bin_attr.size = UCSI_MAILBOX_SIZE(ucsi);
> > > +	sysfs->bin_attr.private = sysfs;
> > > +	sysfs->bin_attr.read_new = ucsi_read;
> > > +	sysfs->bin_attr.write_new = ucsi_write;
> > > +
> > > +	ret = sysfs_create_bin_file(&ucsi->dev->kobj, &sysfs->bin_attr);
> > 
> > You raced with userspace and lost, right?  Why are you dynamically
> > creating this attribute, can't you just use a static one?
> 
> The size of the attribute depends on the UCSI version.
> 
> > But again, why not debugfs?  I'd feel a lot more comfortable with that
> > instead of sysfs.
> 
> I would actually prefer debugfs for this, but this is in any case
> not primarily for debugging and validation.
> 
> The initial goal was to supply the user space some way to control the
> EC's power related policies using UCSI commands such as
> SET_POWER_LEVEL and GET_POWER_LEVEL (guys, please correct me if I got
> that wrong).

It generally feels that exporting the whole unmoderated channel to the
firmware just to set power level is wrong. It should be interfaced
through the PSY driver.

> 
> But I'm now again wondering could those power policy tasks be handled
> using the UCSI power supplies after all? Venkat, did you look into
> that?
> 
> thanks,
> 
> -- 
> heikki
Pathak, Asutosh Feb. 11, 2025, 9:21 p.m. UTC | #3 
On Tue, Feb 11, 2025 at 01:21:28PM -0700, Pathak Asutosh wrote: 
> On Fri, Feb 07, 2025 at 03:04:34PM +0200, Heikki Krogerus wrote:
> > On Thu, Feb 06, 2025 at 03:51:48PM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Feb 06, 2025 at 04:19:31PM +0200, Heikki Krogerus wrote:
> > > > Some of the UCSI commands can be used to configure the
> > > > entire Platform Policy Manager (PPM) instead of just
> > > > individual connectors. To allow the user space communicate
> > > > those commands with the PPM, adding a mailbox interface. The
> > > > interface is a single attribute file that represents the
> > > > main "OPM to PPM" UCSI data structure.
> > > >
> > > > The mailbox allows any UCSI command to be sent to the PPM so
> > > > it should be also useful for validation, testing and
> > > > debugging purposes.
> > >
> > > As it's for this type of thing, why not put it in debugfs instead?

The intend of this sysfs is not limited to validation, testing and
debugging purposes but rather providing interface for major user space
application developments.

At present we are working on an application/ user space service which 
will be calling UCSI read/write power level commands. But in future
there would be more such applications which may require additional
UCSI commands to use. We wanted to have a common and 
generic solution - and hence thought of going with sysfs interface.

Issue with debugfs is, it is default disabled in release kernels. User has 
to rebuild the kernel if the application is based on the debugfs interface.
This will become a bottleneck for wider use of such appliances.
> > >
> > > > +static ssize_t ucsi_write(struct file *filp, struct kobject *kobj,
> > > > +			  const struct bin_attribute *attr,
> > > > +			  char *buf, loff_t off, size_t count)
> > > > +{
> > > > +	struct ucsi_sysfs *sysfs = attr->private;
> > > > +	struct ucsi *ucsi = sysfs->ucsi;
> > > > +	int ret;
> > > > +
> > > > +	u64 *control = (u64 *)&sysfs->mailbox[UCSI_CONTROL];
> > > > +	u32 *cci = (u32 *)&sysfs->mailbox[UCSI_CCI];
> > > > +	void *data = &sysfs->mailbox[UCSI_MESSAGE_IN];
> > > > +
> > > > +	/* TODO: MESSAGE_OUT. */
> > > > +	if (off != UCSI_CONTROL || count != sizeof(*control))
> > > > +		return -EFAULT;
> > > > +
> > > > +	mutex_lock(&sysfs->lock);
> > > > +
> > > > +	memset(data, 0, UCSI_MAX_DATA_LENGTH(ucsi));
> > > > +
> > > > +	/* PPM_RESET has to be handled separately. */
> > > > +	*control = get_unaligned_le64(buf);
> > > > +	if (UCSI_COMMAND(*control) == UCSI_PPM_RESET) {
> > > > +		ret = ucsi_reset_ppm(ucsi, cci);
> > > > +		goto out_unlock_sysfs;
> > > > +	}
> > > > +
> > > > +	mutex_lock(&ucsi->ppm_lock);
> > > > +
> > > > +	ret = ucsi->ops->sync_control(ucsi, *control, cci, NULL, 0);
> > > > +	if (ret)
> > > > +		goto out_unlock_ppm;
> > > > +
> > > > +	if (UCSI_CCI_LENGTH(*cci) && ucsi->ops->read_message_in(ucsi, data,
> UCSI_CCI_LENGTH(*cci)))
> > > > +		dev_err(ucsi->dev, "failed to read MESSAGE_IN\n");
> > > > +
> > > > +	ret = ucsi->ops->sync_control(ucsi, UCSI_ACK_CC_CI |
> UCSI_ACK_COMMAND_COMPLETE,
> > > > +				      NULL, NULL, 0);
> > > > +out_unlock_ppm:
> > > > +	mutex_unlock(&ucsi->ppm_lock);
> > > > +out_unlock_sysfs:
> > > > +	mutex_unlock(&sysfs->lock);
> > > > +
> > > > +	return ret ?: count;
> > > > +}
> > >
> > > This worries me, any userspace tool can now do this?  What other "bad"
> > > things can it to the connection?
> >
> > Although, there is actually only a limited number of things that you
> > can do to the connection using UCSI, that is definitely a concern.
> >
> > The PPM (which is the EC firmware in most cases) is expected to prevent
> > any harmful or "unauthorized" UCSI commands from being executed, but
> > I'm not sure there is any guarantees for that at the moment.
> >
Critical power setting related features and options are tightly controlled 
by PPM/LPM. In such cases, those UCSI command request by user space 
will be blocked by PPM/LPM and will eventually end of into DoS.

Moreover, to further mitigate the risk of any malicious attack our 
understanding is this sysfs interface will be accessible only with root or 
super user privilege. 

> > > > +int ucsi_sysfs_register(struct ucsi *ucsi)
> > > > +{
> > > > +	struct ucsi_sysfs *sysfs;
> > > > +	int ret;
> > > > +
> > > > +	sysfs = kzalloc(struct_size(sysfs, mailbox, UCSI_MAILBOX_SIZE(ucsi)),
> GFP_KERNEL);
> > > > +	if (!sysfs)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	sysfs->ucsi = ucsi;
> > > > +	mutex_init(&sysfs->lock);
> > > > +	memcpy(sysfs->mailbox, &ucsi->version, sizeof(ucsi->version));
> > > > +
> > > > +	sysfs_bin_attr_init(&sysfs->bin_attr);
> > > > +
> > > > +	sysfs->bin_attr.attr.name = "ucsi";
> > > > +	sysfs->bin_attr.attr.mode = 0644;
> > > > +
> > > > +	sysfs->bin_attr.size = UCSI_MAILBOX_SIZE(ucsi);
> > > > +	sysfs->bin_attr.private = sysfs;
> > > > +	sysfs->bin_attr.read_new = ucsi_read;
> > > > +	sysfs->bin_attr.write_new = ucsi_write;
> > > > +
> > > > +	ret = sysfs_create_bin_file(&ucsi->dev->kobj, &sysfs->bin_attr);
> > >
> > > You raced with userspace and lost, right?  Why are you dynamically
> > > creating this attribute, can't you just use a static one?
> >
> > The size of the attribute depends on the UCSI version.
> >
> > > But again, why not debugfs?  I'd feel a lot more comfortable with that
> > > instead of sysfs.
> >
> > I would actually prefer debugfs for this, but this is in any case
> > not primarily for debugging and validation.
> >
> > The initial goal was to supply the user space some way to control the
> > EC's power related policies using UCSI commands such as
> > SET_POWER_LEVEL and GET_POWER_LEVEL (guys, please correct me if I got
> > that wrong).
> 
> It generally feels that exporting the whole unmoderated channel to the
> firmware just to set power level is wrong. It should be interfaced
> through the PSY driver.
> 
> >
> > But I'm now again wondering could those power policy tasks be handled
> > using the UCSI power supplies after all? Venkat, did you look into
> > that?
> >
We are looking into this to figure out if there is any best way to expose 
power level settings options using UCSI power supply class interface. 
But even so, I believe that also does not completely eliminate risk of
any malicious use.

At present we are working on an application/ user space service which 
will be calling UCSI read/write power level commands. But in future
there would be more such applications which may require additional
UCSI commands to use. We wanted to have a common and 
generic solution - and hence thought of going with sysfs interface.

Issue with debugfs is, it is default disabled in release kernels. User has 
to rebuild the kernel if the application is based on the debugfs interface.
This will become a bottleneck for wider use of such appliances.

Can we still think of going ahead with sysfs interface and double make 
sure to make this accessible only with root/su privilege to minimize 
any potential risk of bad uses?

> > thanks,
> >
> > --
> > heikki
> 
> --
> With best wishes
> Dmitry