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  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]
Date:   Thu, 2 Nov 2017 13:12:51 +0200
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Badhri Jagan Sridharan <badhri@...gle.com>
Cc:     gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
        dan.carpenter@...cle.com, Guenter Roeck <linux@...ck-us.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2 v3] typec: tcpm: Validate source and sink caps

+Dan and Guenter,

I don't have any problems with this, but since I had those few
comments for the 2/2 of this series, few nitpics below..

On Wed, Oct 18, 2017 at 01:22:47PM -0700, Badhri Jagan Sridharan wrote:
> The source and sink caps should follow the following rules.
> This patch validates whether the src_caps/snk_caps adheres
> to it.
> 
> 6.4.1 Capabilities Message
> A Capabilities message (Source Capabilities message or Sink
> Capabilities message) shall have at least one Power
> Data Object for vSafe5V. The Capabilities message shall also
> contain the sending Port???s information followed by up to
> 6 additional Power Data Objects. Power Data Objects in a
> Capabilities message shall be sent in the following order:
> 
> 1. The vSafe5V Fixed Supply Object shall always be the first object.
> 2. The remaining Fixed Supply Objects, if present, shall be sent
>    in voltage order; lowest to highest.
> 3. The Battery Supply Objects, if present shall be sent in Minimum
>    Voltage order; lowest to highest.
> 4. The Variable Supply (non-battery) Objects, if present, shall be
>    sent in Minimum Voltage order; lowest to highest.
> 
> Errors in source/sink_caps of the local port will prevent
> the port registration. Whereas, errors in source caps of partner
> device would only log them.
> 
> Signed-off-by: Badhri Jagan Sridharan <Badhri@...gle.com>
> ---
> Changelog since v1:
> - rebased on top drivers/usb/typec/tcpm.c as suggested by
>   gregkh@...uxfoundation.org
> - renamed nr_snk_*_pdo as suggested by dan.carpenter@...cle.com
> - removed stale comment as suggested by linux@...ck-us.net
> - removed the tests for nr_snk_*_pdo as suggested by
>   dan.carpenter@...cle.com
> - Fixed sytling as suggested by dan.carpenter@...cle.com
> - renamed tcpm_get_nr_type_pdos to nr_type_pdos as suggested by
>   dan.carpenter@...cle.com
> - fixed nr_type_pdos as suggested by dan.carpenter@...cle.com
> - tcpm_pd_select_pdo now checks for all matching variable/batt pdos
>   instead of the first matching one.
> 
> Changelog since v2:
> - refactored error messages as suggested by
>   heikki.krogerus@...ux.intel.com
> 
>  drivers/usb/typec/tcpm.c | 116 +++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/usb/pd.h   |   2 +
>  include/linux/usb/tcpm.h |   4 +-
>  3 files changed, 111 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 8483d3e33853..e3f8b3da46b0 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -1256,6 +1256,86 @@ static void vdm_state_machine_work(struct work_struct *work)
>  	mutex_unlock(&port->lock);
>  }
>  
> +static const char * const pdo_err[] = {
> +	"",
> +	" err: source/sink caps should atleast have vSafe5V",
> +	" err: vSafe5V Fixed Supply Object Shall always be the first object",
> +	" err: PDOs should be in the following order: Fixed; Battery; Variable",
> +	" err: Fixed supply pdos should be in increasing order of their fixed voltage",
> +	" err: Variable/Battery supply pdos should be in increasing order of their minimum voltage",
> +	" err: Variable/Batt supply pdos cannot have same min/max voltage",
> +};
> +
> +static int tcpm_caps_err(struct tcpm_port *port, const u32 *pdo,
> +			      unsigned int nr_pdo)

Alignment.

> +{
> +	unsigned int i;
> +
> +	/* Should at least contain vSafe5v */
> +	if (nr_pdo < 1)
> +		return 1;
> +
> +	/* The vSafe5V Fixed Supply Object Shall always be the first object */
> +	if (pdo_type(pdo[0]) != PDO_TYPE_FIXED ||
> +	    pdo_fixed_voltage(pdo[0]) != VSAFE5V)
> +		return 2;
> +
> +	for (i = 1; i < nr_pdo; i++) {
> +		if (pdo_type(pdo[i]) < pdo_type(pdo[i - 1])) {
> +			return 3;
> +		} else if (pdo_type(pdo[i]) == pdo_type(pdo[i - 1])) {
> +			enum pd_pdo_type type = pdo_type(pdo[i]);
> +
> +			switch (type) {
> +			/*
> +			 * The remaining Fixed Supply Objects, if
> +			 * present, shall be sent in voltage order;
> +			 * lowest to highest.
> +			 */
> +			case PDO_TYPE_FIXED:
> +				if (pdo_fixed_voltage(pdo[i]) <=
> +				    pdo_fixed_voltage(pdo[i - 1]))
> +					return 4;
> +				break;
> +			/*
> +			 * The Battery Supply Objects and Variable
> +			 * supply, if present shall be sent in Minimum
> +			 * Voltage order; lowest to highest.
> +			 */
> +			case PDO_TYPE_VAR:
> +			case PDO_TYPE_BATT:
> +				if (pdo_min_voltage(pdo[i]) <
> +				    pdo_min_voltage(pdo[i - 1]))
> +					return 5;
> +				else if ((pdo_min_voltage(pdo[i]) ==
> +					  pdo_min_voltage(pdo[i - 1])) &&
> +					 (pdo_max_voltage(pdo[i]) ==
> +					  pdo_min_voltage(pdo[i - 1])))
> +					return 6;
> +				break;
> +			default:
> +				tcpm_log_force(port, " Unknown pdo type");
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int tcpm_validate_caps(struct tcpm_port *port, const u32 *pdo,
> +			      unsigned int nr_pdo)
> +{
> +	int err_index = tcpm_caps_err(port, pdo, nr_pdo);
> +
> +	if (err_index) {
> +		tcpm_log_force(port, " %s", pdo_err[err_index]);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +

Extra line.

>  /*
>   * PD (data, control) command handling functions
>   */
> @@ -1278,6 +1358,9 @@ static void tcpm_pd_data_request(struct tcpm_port *port,
>  
>  		tcpm_log_source_caps(port);
>  
> +		tcpm_validate_caps(port, port->source_caps,
> +				   port->nr_source_caps);
> +
>  		/*
>  		 * This message may be received even if VBUS is not
>  		 * present. This is quite unexpected; see USB PD
> @@ -3444,9 +3527,12 @@ static int tcpm_copy_vdos(u32 *dest_vdo, const u32 *src_vdo,
>  	return nr_vdo;
>  }
>  
> -void tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
> -				     unsigned int nr_pdo)
> +int tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
> +				    unsigned int nr_pdo)
>  {
> +	if (tcpm_validate_caps(port, pdo, nr_pdo))
> +		return -EINVAL;
> +
>  	mutex_lock(&port->lock);
>  	port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, pdo, nr_pdo);
>  	switch (port->state) {
> @@ -3466,16 +3552,20 @@ void tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
>  		break;
>  	}
>  	mutex_unlock(&port->lock);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tcpm_update_source_capabilities);
>  
> -void tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
> -				   unsigned int nr_pdo,
> -				   unsigned int max_snk_mv,
> -				   unsigned int max_snk_ma,
> -				   unsigned int max_snk_mw,
> -				   unsigned int operating_snk_mw)
> +int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
> +				  unsigned int nr_pdo,
> +				  unsigned int max_snk_mv,
> +				  unsigned int max_snk_ma,
> +				  unsigned int max_snk_mw,
> +				  unsigned int operating_snk_mw)
>  {
> +	if (tcpm_validate_caps(port, pdo, nr_pdo))
> +		return -EINVAL;
> +
>  	mutex_lock(&port->lock);
>  	port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, pdo, nr_pdo);
>  	port->max_snk_mv = max_snk_mv;
> @@ -3494,6 +3584,7 @@ void tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
>  		break;
>  	}
>  	mutex_unlock(&port->lock);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tcpm_update_sink_capabilities);
>  
> @@ -3529,7 +3620,15 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  
>  	init_completion(&port->tx_complete);
>  	init_completion(&port->swap_complete);
> +	tcpm_debugfs_init(port);
>  
> +	if (tcpm_validate_caps(port, tcpc->config->src_pdo,
> +			       tcpc->config->nr_src_pdo) ||
> +	    tcpm_validate_caps(port, tcpc->config->snk_pdo,
> +			       tcpc->config->nr_snk_pdo)) {
> +		err = -EINVAL;
> +		goto out_destroy_wq;
> +	}
>  	port->nr_src_pdo = tcpm_copy_pdos(port->src_pdo, tcpc->config->src_pdo,
>  					  tcpc->config->nr_src_pdo);
>  	port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, tcpc->config->snk_pdo,
> @@ -3584,7 +3683,6 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  		}
>  	}
>  
> -	tcpm_debugfs_init(port);
>  	mutex_lock(&port->lock);
>  	tcpm_init(port);
>  	mutex_unlock(&port->lock);
> diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> index e00051ced806..b3d41d7409b3 100644
> --- a/include/linux/usb/pd.h
> +++ b/include/linux/usb/pd.h
> @@ -148,6 +148,8 @@ enum pd_pdo_type {
>  	(PDO_TYPE(PDO_TYPE_FIXED) | (flags) |		\
>  	 PDO_FIXED_VOLT(mv) | PDO_FIXED_CURR(ma))
>  
> +#define VSAFE5V 5000 /* mv units */
> +
>  #define PDO_BATT_MAX_VOLT_SHIFT	20	/* 50mV units */
>  #define PDO_BATT_MIN_VOLT_SHIFT	10	/* 50mV units */
>  #define PDO_BATT_MAX_PWR_SHIFT	0	/* 250mW units */
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index 073197f0d2bb..bc419f93c90e 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -183,9 +183,9 @@ struct tcpm_port;
>  struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc);
>  void tcpm_unregister_port(struct tcpm_port *port);
>  
> -void tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
> +int tcpm_update_source_capabilities(struct tcpm_port *port, const u32 *pdo,
>  				     unsigned int nr_pdo);
> -void tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
> +int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
>  				   unsigned int nr_pdo,
>  				   unsigned int max_snk_mv,
>  				   unsigned int max_snk_ma,

I think that's also misaligned.

You can run the checkpatch.pl script with --strict option and it will
check also things like alignment and extra lines.


Thanks,

-- 
heikki

Powered by blists - more mailing lists