Re: [PATCH v3 2/2] usb: typec: ucsi: Implement ChromeOS UCSI driver

[Greg Kroah-Hartman]

On Wed, Apr 03, 2024 at 09:58:33PM +0300, Dmitry Baryshkov wrote:
> On Wed, Apr 03, 2024 at 06:05:22PM +0000, Pavan Holla wrote:
> > Implementation of a UCSI transport driver for ChromeOS.
> > This driver will be loaded if the ChromeOS EC implements a PPM.
> > 
> > Signed-off-by: Pavan Holla <pholla@xxxxxxxxxxxx>
> > ---
> >  drivers/usb/typec/ucsi/Kconfig        |  13 ++
> >  drivers/usb/typec/ucsi/Makefile       |   1 +
> >  drivers/usb/typec/ucsi/cros_ec_ucsi.c | 245 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 259 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> > index bdcb1764cfae..4dceb14a66ee 100644
> > --- a/drivers/usb/typec/ucsi/Kconfig
> > +++ b/drivers/usb/typec/ucsi/Kconfig
> > @@ -69,4 +69,17 @@ config UCSI_PMIC_GLINK
> >  	  To compile the driver as a module, choose M here: the module will be
> >  	  called ucsi_glink.
> >  
> > +config CROS_EC_UCSI
> > +	tristate "UCSI Driver for ChromeOS EC"
> > +	depends on MFD_CROS_EC_DEV
> > +	depends on CROS_USBPD_NOTIFY
> > +	depends on !EXTCON_TCSS_CROS_EC
> > +	default MFD_CROS_EC_DEV
> > +	help
> > +	  This driver enables UCSI support for a ChromeOS EC. The EC is
> > +	  expected to implement a PPM.
> > +
> > +	  To compile the driver as a module, choose M here: the module
> > +	  will be called cros_ec_ucsi.
> > +
> >  endif
> > diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> > index b4679f94696b..cb336eee055c 100644
> > --- a/drivers/usb/typec/ucsi/Makefile
> > +++ b/drivers/usb/typec/ucsi/Makefile
> > @@ -21,3 +21,4 @@ obj-$(CONFIG_UCSI_ACPI)			+= ucsi_acpi.o
> >  obj-$(CONFIG_UCSI_CCG)			+= ucsi_ccg.o
> >  obj-$(CONFIG_UCSI_STM32G0)		+= ucsi_stm32g0.o
> >  obj-$(CONFIG_UCSI_PMIC_GLINK)		+= ucsi_glink.o
> > +obj-$(CONFIG_CROS_EC_UCSI)		+= cros_ec_ucsi.o
> > diff --git a/drivers/usb/typec/ucsi/cros_ec_ucsi.c b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
> > new file mode 100644
> > index 000000000000..dd46b46d430f
> > --- /dev/null
> > +++ b/drivers/usb/typec/ucsi/cros_ec_ucsi.c
> > @@ -0,0 +1,245 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * UCSI driver for ChromeOS EC
> > + *
> > + * Copyright 2024 Google LLC.
> > + */
> > +
> > +#include <linux/container_of.h>
> > +#include <linux/dev_printk.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_data/cros_ec_commands.h>
> > +#include <linux/platform_data/cros_usbpd_notify.h>
> > +#include <linux/platform_data/cros_ec_proto.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/wait.h>
> > +
> > +#include "ucsi.h"
> > +
> > +#define DRV_NAME "cros-ec-ucsi"
> > +
> > +#define MAX_EC_DATA_SIZE 256
> > +#define WRITE_TMO_MS 500
> > +
> > +struct cros_ucsi_data {
> > +	struct device *dev;
> > +	struct ucsi *ucsi;
> > +
> > +	struct cros_ec_device *ec;
> > +	struct notifier_block nb;
> > +	struct work_struct work;
> > +
> > +	struct completion complete;
> > +	unsigned long flags;
> > +};
> > +
> > +static int cros_ucsi_read(struct ucsi *ucsi, unsigned int offset, void *val,
> > +			  size_t val_len)
> > +{
> > +	struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
> > +	struct ec_params_ucsi_ppm_get req = {
> > +		.offset = offset,
> > +		.size = val_len,
> > +	};
> > +	int ret;
> > +
> > +	if (val_len > MAX_EC_DATA_SIZE) {
> > +		dev_err(udata->dev, "Can't read %zu bytes. Too big.", val_len);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = cros_ec_cmd(udata->ec, 0, EC_CMD_UCSI_PPM_GET,
> > +			  &req, sizeof(req), val, val_len);
> > +	if (ret < 0) {
> > +		dev_warn(udata->dev, "Failed to send EC message UCSI_PPM_GET: error=%d", ret);
> > +		return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int cros_ucsi_async_write(struct ucsi *ucsi, unsigned int offset,
> > +				 const void *val, size_t val_len)
> > +{
> > +	struct cros_ucsi_data *udata = ucsi_get_drvdata(ucsi);
> > +	uint8_t ec_buffer[MAX_EC_DATA_SIZE + sizeof(struct ec_params_ucsi_ppm_set)];
> > +	struct ec_params_ucsi_ppm_set *req = (struct ec_params_ucsi_ppm_set *)ec_buffer;
> > +	int ret = 0;
> > +
> > +	if (val_len > MAX_EC_DATA_SIZE) {
> > +		dev_err(udata->dev, "Can't write %zu bytes. Too big.", val_len);
> 
> I think it's better be written as
> 
> if (WARN_ON_ONCE(val_len > MAX_EC_DATA_SIZE))
> 	return -EINVAL;

So if you trigger this, you just rebooted all boxes that have
panic-on-warn enabled (hint, the HUGE majority in quantity of Linux
systems out there.)

So don't do that, just handle it like this.

BUT, if this can be triggered by userspace, do NOT use dev_err() as that
will just allow userspace to flood the kernel log.

Pavan, who calls this?  If userspace, this needs to be fixed.  If it's
only a kernel driver, it's fine as-is.

thanks,

greg k-h