From 1620833b5e7c97a1f0d2a7007746e1f961d7587f Mon Sep 17 00:00:00 2001 From: Wai-Hong Tam Date: Wed, 24 Jun 2020 10:02:45 -0700 Subject: [PATCH] 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 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2264477 Reviewed-by: Julius Werner --- common/mock/tcpc_mock.c | 2 +- common/usb_pd_host_cmd.c | 4 ++-- common/usb_pd_protocol.c | 6 +++--- driver/tcpm/anx74xx.c | 10 +++++----- driver/tcpm/it83xx.c | 14 ++++++-------- driver/tcpm/it8xxx2.c | 14 ++++++-------- driver/tcpm/ps8xxx.c | 20 ++++++++++---------- driver/tcpm/tcpci.c | 24 ++++++++++++++++-------- driver/tcpm/tcpci.h | 2 +- driver/tcpm/tcpm.h | 2 +- fuzz/usb_pd_fuzz.c | 2 +- include/usb_pd_tcpm.h | 4 ++-- 12 files changed, 54 insertions(+), 50 deletions(-) diff --git a/common/mock/tcpc_mock.c b/common/mock/tcpc_mock.c index 8d62710f6a..27b79af989 100644 --- a/common/mock/tcpc_mock.c +++ b/common/mock/tcpc_mock.c @@ -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; } diff --git a/common/usb_pd_host_cmd.c b/common/usb_pd_host_cmd.c index 6e8afe761e..3a26018e64 100644 --- a/common/usb_pd_host_cmd.c +++ b/common/usb_pd_host_cmd.c @@ -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; } diff --git a/common/usb_pd_protocol.c b/common/usb_pd_protocol.c index 151e4b50aa..31093b9a2d 100644 --- a/common/usb_pd_protocol.c +++ b/common/usb_pd_protocol.c @@ -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 diff --git a/driver/tcpm/anx74xx.c b/driver/tcpm/anx74xx.c index 26749891c9..06667ab5ae 100644 --- a/driver/tcpm/anx74xx.c +++ b/driver/tcpm/anx74xx.c @@ -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; diff --git a/driver/tcpm/it83xx.c b/driver/tcpm/it83xx.c index 360799061d..f6e72e4d92 100644 --- a/driver/tcpm/it83xx.c +++ b/driver/tcpm/it83xx.c @@ -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; } diff --git a/driver/tcpm/it8xxx2.c b/driver/tcpm/it8xxx2.c index f70a740bd0..82a903a272 100644 --- a/driver/tcpm/it8xxx2.c +++ b/driver/tcpm/it8xxx2.c @@ -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; } diff --git a/driver/tcpm/ps8xxx.c b/driver/tcpm/ps8xxx.c index 9349f2e7c5..4126ed24a0 100644 --- a/driver/tcpm/ps8xxx.c +++ b/driver/tcpm/ps8xxx.c @@ -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; diff --git a/driver/tcpm/tcpci.c b/driver/tcpm/tcpci.c index ea1fdccb38..d714b4d636 100644 --- a/driver/tcpm/tcpci.c +++ b/driver/tcpm/tcpci.c @@ -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; } diff --git a/driver/tcpm/tcpci.h b/driver/tcpm/tcpci.h index 73e847de6e..d728b55332 100644 --- a/driver/tcpm/tcpci.h +++ b/driver/tcpm/tcpci.h @@ -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); diff --git a/driver/tcpm/tcpm.h b/driver/tcpm/tcpm.h index 2c53e03d5d..98861a78f1 100644 --- a/driver/tcpm/tcpm.h +++ b/driver/tcpm/tcpm.h @@ -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); diff --git a/fuzz/usb_pd_fuzz.c b/fuzz/usb_pd_fuzz.c index 1c0a40f225..ab0ab2439e 100644 --- a/fuzz/usb_pd_fuzz.c +++ b/fuzz/usb_pd_fuzz.c @@ -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; } diff --git a/include/usb_pd_tcpm.h b/include/usb_pd_tcpm.h index 9cfc1c9908..0441af6299 100644 --- a/include/usb_pd_tcpm.h +++ b/include/usb_pd_tcpm.h @@ -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 /**