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
