linux-kernel - Re: [PATCH v4 2/2] usb: typec: ucsi: Implement ChromeOS UCSI driver

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2024040916-kleenex-machinist-4858@gregkh>
Date: Tue, 9 Apr 2024 17:16:42 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Pavan Holla <pholla@...omium.org>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Benson Leung <bleung@...omium.org>,
	Tzung-Bi Shih <tzungbi@...nel.org>,
	Guenter Roeck <groeck@...omium.org>, linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org,
	Abhishek Pandit-Subedi <abhishekpandit@...omium.org>,
	chrome-platform@...ts.linux.dev
Subject: Re: [PATCH v4 2/2] usb: typec: ucsi: Implement ChromeOS UCSI driver

On Tue, Apr 09, 2024 at 02:27:37AM +0000, Pavan Holla wrote:
> +#define DRV_NAME "cros-ec-ucsi"

KBUILD_MODNAME?

> +
> +#define MAX_EC_DATA_SIZE 256
> +#define WRITE_TMO_MS 500

What are these and why these values?  And tabs perhaps?

> +
> +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)];

That's a lot of data on the stack, are you sure you want to do that?

And are you sure you are allowed to have this data on the stack?  It
never ends up being sent using DMA?

And please, don't use non-kernel types like "uint8_t", use "u8" like
intended.  This isn't userspace (yes, I know a lot of kernel code uses
these, but as you are going to change this, might as well change that
too.)

thanks,

greg k-h