coreboot-mirror
/
chrome-ec
mirror of https://review.coreboot.org/chrome-ec.git
1
0
Fork 0
Code Issues Releases Wiki Activity

tcpm: Change the get_chip_info() to prevent race conditions 

The original get_chip_info() returns a point of point to the chip_info.
This way helps to cache the chip_info to a static variable and the
function just returns the pointer to the static variable.

This static variable has a race condition on the PS8805 chip. The PS8805
chip returns a different PID when the firmware is corrupted, i.e. 0x8803
instead of 0x8805. The !live case fixes the PID, by modifying the static
variable directly. When another task calls the same function for the
live case, the static variable is modified and has a race condition.

This change fixes the issue by changing the get_chip_info() parameter to
a point of the chip_info. The caller has to allocate a buffer in the
stack and pass the address to the function. For the !live case, the
function copies the cache value from the static variable to the buffer.
So the static variable doesn't have a race condition.

BRANCH=None
BUG=b:159588335
TEST=Used ectool to check the PD chip PID 0x8805 (was 0x8803).
localhost ~ # ectool pdchipinfo 1
vendor_id: 0x1da0
product_id: 0x8805
device_id: 0x1
fw_version: 0x0
min_req_fw_version: 0x0

Change-Id: Ic24615af77ea58016d286480572d2a282c4fa09a
Signed-off-by: Wai-Hong Tam <waihong@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2264477
Reviewed-by: Julius Werner <jwerner@chromium.org>
This commit is contained in:
Wai-Hong Tam 2020-06-24 10:02:45 -07:00 committed by Commit Bot
parent d945012134
commit 1620833b5e
12 changed files with 54 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
common/mock/tcpc_mock.c
View File

@ -146,7 +146,7 @@ __maybe_unused static int mock_drp_toggle(int port)
}
static int mock_get_chip_info(int port, int live,
struct ec_response_pd_chip_info_v1 **info)
struct ec_response_pd_chip_info_v1 *info)
{
return EC_SUCCESS;
}

4
common/usb_pd_host_cmd.c
View File

@ -89,7 +89,7 @@ DECLARE_HOST_COMMAND(EC_CMD_USB_PD_RW_HASH_ENTRY,
static enum ec_status hc_remote_pd_chip_info(struct host_cmd_handler_args *args)
{
const struct ec_params_pd_chip_info *p = args->params;
struct ec_response_pd_chip_info_v1 *info;
struct ec_response_pd_chip_info_v1 info;
if (p->port >= board_get_usb_pd_port_count())
return EC_RES_INVALID_PARAM;
@ -105,7 +105,7 @@ static enum ec_status hc_remote_pd_chip_info(struct host_cmd_handler_args *args)
args->version ? sizeof(struct ec_response_pd_chip_info_v1)
: sizeof(struct ec_response_pd_chip_info);
memcpy(args->response, info, args->response_size);
memcpy(args->response, &info, args->response_size);
return EC_RES_SUCCESS;
}

6
common/usb_pd_protocol.c
View File

@ -2940,14 +2940,14 @@ void pd_task(void *u)
this_state = res ? PD_STATE_SUSPENDED : PD_DEFAULT_STATE(port);
#ifndef CONFIG_USB_PD_TCPC
if (!res) {
struct ec_response_pd_chip_info_v1 *info;
struct ec_response_pd_chip_info_v1 info;
if (tcpm_get_chip_info(port, 0, &info) ==
EC_SUCCESS) {
CPRINTS("TCPC p%d VID:0x%x PID:0x%x DID:0x%x "
"FWV:0x%" PRIx64,
port, info->vendor_id, info->product_id,
info->device_id, info->fw_version_number);
port, info.vendor_id, info.product_id,
info.device_id, info.fw_version_number);
}
}
#endif

10
driver/tcpm/anx74xx.c
View File

@ -1114,7 +1114,7 @@ static int anx74xx_tcpm_init(int port)
}
static int anx74xx_get_chip_info(int port, int live,
struct ec_response_pd_chip_info_v1 **chip_info)
struct ec_response_pd_chip_info_v1 *chip_info)
{
int rv = tcpci_get_chip_info(port, live, chip_info);
int val;
@ -1122,14 +1122,14 @@ static int anx74xx_get_chip_info(int port, int live,
if (rv)
return rv;
if ((*chip_info)->fw_version_number == 0 ||
(*chip_info)->fw_version_number == -1 || live) {
if (chip_info->fw_version_number == 0 ||
chip_info->fw_version_number == -1 || live) {
rv = tcpc_read(port, ANX74XX_REG_FW_VERSION, &val);
if (rv)
return rv;
(*chip_info)->fw_version_number = val;
chip_info->fw_version_number = val;
}
#ifdef CONFIG_USB_PD_TCPM_ANX3429
@ -1138,7 +1138,7 @@ static int anx74xx_get_chip_info(int port, int live,
* doesn't occur for e-marked cables. See b/116255749#comment8 and
* b/64752060#comment11
*/
(*chip_info)->min_req_fw_version_number = 0x16;
chip_info->min_req_fw_version_number = 0x16;
#endif
return rv;

14
driver/tcpm/it83xx.c
View File

@ -614,15 +614,13 @@ static int it83xx_tcpm_transmit(int port,
}
static int it83xx_tcpm_get_chip_info(int port, int live,
struct ec_response_pd_chip_info_v1 **chip_info)
struct ec_response_pd_chip_info_v1 *chip_info)
{
static struct ec_response_pd_chip_info_v1 i;
*chip_info = &i;
i.vendor_id = USB_VID_ITE;
i.product_id = (IT83XX_GCTRL_CHIPID1 << 8) | IT83XX_GCTRL_CHIPID2;
i.device_id = IT83XX_GCTRL_CHIPVER & 0xf;
i.fw_version_number = 0xEC;
chip_info->vendor_id = USB_VID_ITE;
chip_info->product_id = ((IT83XX_GCTRL_CHIPID1 << 8) |
IT83XX_GCTRL_CHIPID2);
chip_info->device_id = IT83XX_GCTRL_CHIPVER & 0xf;
chip_info->fw_version_number = 0xEC;
return EC_SUCCESS;
}

14
driver/tcpm/it8xxx2.c
View File

@ -574,15 +574,13 @@ static int it83xx_tcpm_transmit(int port,
}
static int it83xx_tcpm_get_chip_info(int port, int live,
struct ec_response_pd_chip_info_v1 **chip_info)
struct ec_response_pd_chip_info_v1 *chip_info)
{
static struct ec_response_pd_chip_info_v1 i;
*chip_info = &i;
i.vendor_id = USB_VID_ITE;
i.product_id = (IT83XX_GCTRL_CHIPID1 << 8) | IT83XX_GCTRL_CHIPID2;
i.device_id = IT83XX_GCTRL_CHIPVER & 0xf;
i.fw_version_number = 0xEC;
chip_info->vendor_id = USB_VID_ITE;
chip_info->product_id = ((IT83XX_GCTRL_CHIPID1 << 8) |
IT83XX_GCTRL_CHIPID2);
chip_info->device_id = IT83XX_GCTRL_CHIPVER & 0xf;
chip_info->fw_version_number = 0xEC;
return EC_SUCCESS;
}

20
driver/tcpm/ps8xxx.c
View File

@ -186,7 +186,7 @@ static int ps8xxx_tcpc_drp_toggle(int port)
static int ps8xxx_get_chip_info(int port, int live,
struct ec_response_pd_chip_info_v1 **chip_info)
struct ec_response_pd_chip_info_v1 *chip_info)
{
int val;
int rv = tcpci_get_chip_info(port, live, chip_info);
@ -195,25 +195,25 @@ static int ps8xxx_get_chip_info(int port, int live,
return rv;
if (!live) {
(*chip_info)->vendor_id = PS8XXX_VENDOR_ID;
(*chip_info)->product_id = PS8XXX_PRODUCT_ID;
chip_info->vendor_id = PS8XXX_VENDOR_ID;
chip_info->product_id = PS8XXX_PRODUCT_ID;
}
if ((*chip_info)->fw_version_number == 0 ||
(*chip_info)->fw_version_number == -1 || live) {
if (chip_info->fw_version_number == 0 ||
chip_info->fw_version_number == -1 || live) {
rv = tcpc_read(port, FW_VER_REG, &val);
if (rv)
return rv;
(*chip_info)->fw_version_number = val;
chip_info->fw_version_number = val;
}
/* Treat unexpected values as error (FW not initiated from reset) */
if (live && (
(*chip_info)->vendor_id != PS8XXX_VENDOR_ID ||
(*chip_info)->product_id != PS8XXX_PRODUCT_ID ||
(*chip_info)->fw_version_number == 0))
chip_info->vendor_id != PS8XXX_VENDOR_ID ||
chip_info->product_id != PS8XXX_PRODUCT_ID ||
chip_info->fw_version_number == 0))
return EC_ERROR_UNKNOWN;
#if defined(CONFIG_USB_PD_TCPM_PS8751) && \
@ -222,7 +222,7 @@ static int ps8xxx_get_chip_info(int port, int live,
* Min firmware version of PS8751 to ensure that it can detect Vbus
* properly. See b/109769787#comment7
*/
(*chip_info)->min_req_fw_version_number = 0x39;
chip_info->min_req_fw_version_number = 0x39;
#endif
return rv;

24
driver/tcpm/tcpci.c
View File

@ -1214,10 +1214,10 @@ void tcpci_tcpc_alert(int port)
* accessed by tcpm_get_chip_info without worrying about chip states.
*/
int tcpci_get_chip_info(int port, int live,
struct ec_response_pd_chip_info_v1 **chip_info)
struct ec_response_pd_chip_info_v1 *chip_info)
{
static struct ec_response_pd_chip_info_v1
info[CONFIG_USB_PD_PORT_MAX_COUNT];
cached_info[CONFIG_USB_PD_PORT_MAX_COUNT];
struct ec_response_pd_chip_info_v1 *i;
int error;
int val;
@ -1225,16 +1225,20 @@ int tcpci_get_chip_info(int port, int live,
if (port >= board_get_usb_pd_port_count())
return EC_ERROR_INVAL;
i = &info[port];
i = &cached_info[port];
/* If chip_info is NULL, chip info will be stored in cache and can be
* read later by another call. */
if (chip_info)
*chip_info = i;
/* If already cached && live data is not asked, return cached value */
if (i->vendor_id && !live)
if (i->vendor_id && !live) {
/*
* If chip_info is NULL, chip info will be stored in cache and
* can be read later by another call.
*/
if (chip_info)
memcpy(chip_info, i, sizeof(*i));
return EC_SUCCESS;
}
error = tcpc_read16(port, TCPC_REG_VENDOR_ID, &val);
if (error)
@ -1257,6 +1261,10 @@ int tcpci_get_chip_info(int port, int live,
*/
i->fw_version_number = -1;
/* Copy the cached value to return if chip_info is not NULL */
if (chip_info)
memcpy(chip_info, i, sizeof(*i));
return EC_SUCCESS;
}

2
driver/tcpm/tcpci.h
View File

@ -239,7 +239,7 @@ int tcpci_tcpm_mux_init(const struct usb_mux *me);
int tcpci_tcpm_mux_set(const struct usb_mux *me, mux_state_t mux_state);
int tcpci_tcpm_mux_get(const struct usb_mux *me, mux_state_t *mux_state);
int tcpci_get_chip_info(int port, int live,
struct ec_response_pd_chip_info_v1 **chip_info);
struct ec_response_pd_chip_info_v1 *chip_info);
#ifdef CONFIG_USBC_PPC
int tcpci_tcpm_set_snk_ctrl(int port, int enable);
int tcpci_tcpm_set_src_ctrl(int port, int enable);

2
driver/tcpm/tcpm.h
View File

@ -296,7 +296,7 @@ static inline int tcpc_i2c_write(const int port, const uint16_t addr_flags,
#endif
static inline int tcpm_get_chip_info(int port, int live,
struct ec_response_pd_chip_info_v1 **info)
struct ec_response_pd_chip_info_v1 *info)
{
if (tcpc_config[port].drv->get_chip_info)
return tcpc_config[port].drv->get_chip_info(port, live, info);

2
fuzz/usb_pd_fuzz.c
View File

@ -44,7 +44,7 @@ static int mock_tcpm_transmit(int port, enum tcpm_transmit_type type,
uint16_t header, const uint32_t *data) { return EC_SUCCESS; }
static void mock_tcpc_alert(int port) {}
static int mock_tcpci_get_chip_info(int port, int live,
struct ec_response_pd_chip_info_v1 **info)
struct ec_response_pd_chip_info_v1 *info)
{
return EC_ERROR_UNIMPLEMENTED;
}

4
include/usb_pd_tcpm.h
View File

@ -344,12 +344,12 @@ struct tcpm_drv {
*
* @param port Type-C port number
* @param live Fetch live chip info or hard-coded + cached info
* @param info Pointer to pointer to PD chip info
* @param info Pointer to PD chip info; NULL to cache the info only
*
* @return EC_SUCCESS or error
*/
int (*get_chip_info)(int port, int live,
struct ec_response_pd_chip_info_v1 **info);
struct ec_response_pd_chip_info_v1 *info);
#ifdef CONFIG_USBC_PPC
/**