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>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1709612
Reviewed-by: Edward Hill <ecgh@chromium.org>
Commit-Queue: Edward Hill <ecgh@chromium.org>
Tested-by: Edward Hill <ecgh@chromium.org>

1 files changed, 16 insertions, 11 deletions

diff --git a/common/usb_pd_protocol.c b/common/usb_pd_protocol.c
index 8cef253f20..eb76bb091a 100644
--- a/common/usb_pd_protocol.c
+++ b/common/usb_pd_protocol.c
@@ -2304,25 +2304,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;
 
 	/*