diff options
author | Nicolas Boichat <drinkcat@chromium.org> | 2019-05-08 13:38:19 +0900 |
---|---|---|
committer | chrome-bot <chrome-bot@chromium.org> | 2019-05-12 19:27:49 -0700 |
commit | 037fdf6599bb0e78a498b1b5a3d0dcc1782e6b7c (patch) | |
tree | c5a578800c7c8e44e45a552c58eec2c716925010 /common | |
parent | 54379d5baf08425b06e263a7d592f02fbc2aa317 (diff) | |
download | chrome-ec-037fdf6599bb0e78a498b1b5a3d0dcc1782e6b7c.tar.gz |
common/usb_pd_protocol: Avoid unitialized use of port flags
On reset, when pd_partner_port_reset is called, if, for any reason
pd_get_saved_port_flags fails to get the saved port status, we
would risk using unitialized flags from the stack.
The function logic was a little complicated, and can be simplified
quite a bit by returning early if no contract is in place (or if
we fail to read flags from BBRAM).
This should not be a problem on real boards, as the stored value
should be readable from BBRAM.
In any case, the first time we used the flag
if (explicit_contract_in_place && pd_comm_is_enabled(port))
only applies to unlocked RO images, so this never happens in production
(where RO images are locked).
The second case is a little tricker, and we may end up (not) applying
Rp where we should (not):
/* If we just lost power, don't apply Rp. */
if (!explicit_contract_in_place ||
system_get_reset_flags() &
(RESET_FLAG_BROWNOUT | RESET_FLAG_POWER_ON))
return;
Presumably, the worst case here is that a charger may not work after
a brownout/power on.
BRANCH=none
BUG=chromium:958510
TEST=setup_board --board=amd64-generic --profile=msan-fuzzer
cros_fuzz --board=amd64-generic reproduce --fuzzer \
chromeos_ec_usb_pd_fuzzer --testcase \
./clusterfuzz-testcase-minimized-ec_usb_pd_fuzzer-5086219896225792 \
--package chromeos-ec --build-type msan
=> No more MemorySanitizer error.
Change-Id: I40fb87b68dbe5244e8a2ae136508b431db7f96a8
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1600935
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Jett Rink <jettrink@chromium.org>
Reviewed-by: Daisuke Nojiri <dnojiri@chromium.org>
Diffstat (limited to 'common')
-rw-r--r-- | common/usb_pd_protocol.c | 27 |
1 files changed, 16 insertions, 11 deletions
diff --git a/common/usb_pd_protocol.c b/common/usb_pd_protocol.c index f3f990bb25..303d56f0d2 100644 --- a/common/usb_pd_protocol.c +++ b/common/usb_pd_protocol.c @@ -2351,25 +2351,30 @@ static int pd_is_power_swapping(int port) static void pd_partner_port_reset(int port) { uint64_t timeout; - int explicit_contract_in_place; uint8_t flags; - pd_get_saved_port_flags(port, &flags); - explicit_contract_in_place = (flags & PD_BBRMFLG_EXPLICIT_CONTRACT); + /* + * If there is no contract in place (or if we fail to read the BBRAM + * flags), there is no need to reset the partner. + */ + if (pd_get_saved_port_flags(port, &flags) != EC_SUCCESS || + !(flags & PD_BBRMFLG_EXPLICIT_CONTRACT)) + return; /* - * If an explicit contract is in place and PD communications are - * allowed, don't apply Rp. We'll issue a SoftReset later on and - * renegotiate our contract. This particular condition only applies to - * unlocked RO images with an explicit contract in place. + * If we reach here, an explicit contract is in place. + * + * If PD communications are allowed, don't apply Rp. We'll issue a + * SoftReset later on and renegotiate our contract. This particular + * condition only applies to unlocked RO images with an explicit + * contract in place. */ - if (explicit_contract_in_place && pd_comm_is_enabled(port)) + if (pd_comm_is_enabled(port)) return; /* If we just lost power, don't apply Rp. */ - if (!explicit_contract_in_place || - system_get_reset_flags() & - (RESET_FLAG_BROWNOUT | RESET_FLAG_POWER_ON)) + if (system_get_reset_flags() & + (RESET_FLAG_BROWNOUT | RESET_FLAG_POWER_ON)) return; /* |