linux-kernel - Re: [PATCH v1 1/2] usb: typec: ucsi: Command mailbox interface for the userspace

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: <mpe6hgyqmw5eohrwulzy7ujdrlgccgqxwdjdjq7zmsdhsemzcd@b6q2hu5ezvqv>
Date: Fri, 7 Feb 2025 22:15:50 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Łukasz Bartosik <ukaszb@...omium.org>, Abhishek Pandit-Subedi <abhishekpandit@...omium.org>, 
	Benson Leung <bleung@...omium.org>, Pavan Holla <pholla@...omium.org>, 
	"Christian A. Ehrhardt" <lk@...e.de>, Jameson Thies <jthies@...gle.com>, 
	"Katiyar, Pooja" <pooja.katiyar@...el.com>, "Pathak, Asutosh" <asutosh.pathak@...el.com>, 
	"Jayaraman, Venkat" <venkat.jayaraman@...el.com>, linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] usb: typec: ucsi: Command mailbox interface for
 the userspace

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

-- 
With best wishes
Dmitry