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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, Feb 11, 2025 at 09:21:28PM +0000, Pathak, Asutosh wrote:
> 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.

But that's not what you are saying above.  sysfs is for attributes of a
device, NOT for full device control.  Use a proper api for that that can
be correctly mediated if needed.

> 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.

We can't take new user/kernel apis without a real user, so please hold
off on this series until you have a real user.  Otherwise it is
guaranteed that you will have to change that api based on actually using
it.

> 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.

It is up to the distro to enable/disable debugfs, that's not our issue.
debugfs is NOT for normal system operation, so if you want to make this
a proper api for normal users, than no, don't use debugfs, make it a
real api.  Which is probably NOT going to be sysfs.

> > > > > +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.

What is "PPM/LPM"?  I don't see that here.

> 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. 

Is it?  You really want normal users being forced to be root in order to
talk to this device?

Make this a real api please, don't try to just do "provide raw access to
the hardware and we will hope any userspace program can get it right",
that way lies madness :)

> 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?

Nope!  Get it right please, once you add it, you can't remove it.

thanks,

greg k-h