Change in coreboot[master]: chromeos/cr50_enable_update.c: Modify recovery flow for cr50

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Edward O'Callaghan (Code Review)

2 Jun 2020 2 Jun '20

7:59 a.m.

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41988 )

Change subject: chromeos/cr50_enable_update.c: Modify recovery flow for cr50 ......................................................................

chromeos/cr50_enable_update.c: Modify recovery flow for cr50

Enable Cr50 update in recovery mode, so that we can at least still update the process for most cases (that an update is pending in recovery mode is not impossible but should be unlikely in the field).

Leave manual recovery unaffected so at least that would still work even if Cr50 wedges in a weird way that it thinks it has an update on every boot or something.

Setting the recovery_reason to VB2_RECOVERY_TRAIN_AND_REBOOT allows the update to be applied.

BUG=b:154071064 BRANCH=none TEST=builds

Thanks to Julius Werner for the suggested fix.

Change-Id: Iba341a750cce8334da4dcf6353ca8cd1268d170f Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/vendorcode/google/chromeos/cr50_enable_update.c 1 file changed, 2 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/41988/1

diff --git a/src/vendorcode/google/chromeos/cr50_enable_update.c b/src/vendorcode/google/chromeos/cr50_enable_update.c index 5b3a1a3..4351e7c 100644 --- a/src/vendorcode/google/chromeos/cr50_enable_update.c +++ b/src/vendorcode/google/chromeos/cr50_enable_update.c @@ -73,7 +73,8 @@ uint8_t num_restored_headers;

/* Nothing to do on recovery mode. */ - if (vboot_recovery_mode_enabled()) + if (vboot_recovery_mode_enabled() && + (vboot_get_context()->flags & VB2_CONTEXT_FORCE_RECOVERY_MODE)) return;

ret = tlcl_lib_init();

Attachments:

attachment.htm (text/html — 3.2 KB)

Show replies by date

Furquan Shaikh (Code Review)

2 Jun 2 Jun

8:43 a.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41988 )

Change subject: chromeos/cr50_enable_update.c: Modify recovery flow for cr50 ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41988/1/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/cr50_enable_update.c:

https://review.coreboot.org/c/coreboot/+/41988/1/src/vendorcode/google/chrom... PS1, Line 77: (vboot_get_context()->flags & VB2_CONTEXT_FORCE_RECOVERY_MODE)) Can you please add a comment here explaining the reason behind doing this?

-- To view, visit https://review.coreboot.org/c/coreboot/+/41988 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iba341a750cce8334da4dcf6353ca8cd1268d170f Gerrit-Change-Number: 41988 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 02 Jun 2020 06:43:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Julius Werner (Code Review)

10:44 p.m.

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41988 )

Change subject: chromeos/cr50_enable_update.c: Modify recovery flow for cr50 ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41988/1/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/cr50_enable_update.c:

https://review.coreboot.org/c/coreboot/+/41988/1/src/vendorcode/google/chrom... PS1, Line 76: vboot_recovery_mode_enabled nit: This check is redundant because the other one already implies this.

-- To view, visit https://review.coreboot.org/c/coreboot/+/41988 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iba341a750cce8334da4dcf6353ca8cd1268d170f Gerrit-Change-Number: 41988 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 02 Jun 2020 20:44:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

3 Jun 3 Jun

2:52 a.m.

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41988 )

Change subject: chromeos/cr50_enable_update.c: Modify recovery flow for cr50 ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41988/1/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/cr50_enable_update.c:

https://review.coreboot.org/c/coreboot/+/41988/1/src/vendorcode/google/chrom... PS1, Line 76: vboot_recovery_mode_enabled

...

nit: This check is redundant because the other one already implies this.

Done

https://review.coreboot.org/c/coreboot/+/41988/1/src/vendorcode/google/chrom... PS1, Line 77: (vboot_get_context()->flags & VB2_CONTEXT_FORCE_RECOVERY_MODE))

...

Can you please add a comment here explaining the reason behind doing this?

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/41988 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iba341a750cce8334da4dcf6353ca8cd1268d170f Gerrit-Change-Number: 41988 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 03 Jun 2020 00:52:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Julius Werner jwerner@chromium.org Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

2:54 a.m.

Hello Sam McNally, build bot (Jenkins), Furquan Shaikh, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41988

to look at the new patch set (#2).

Change subject: chromeos/cr50_enable_update.c: Modify recovery flow for cr50 ......................................................................

chromeos/cr50_enable_update.c: Modify recovery flow for cr50

Leave manual recovery unaffected so at least that would still work even if Cr50 wedges in a weird way that it thinks it has an update on every boot or something.

Setting the recovery_reason to VB2_RECOVERY_TRAIN_AND_REBOOT allows the update to be applied.

BUG=b:154071064 BRANCH=none TEST=builds

Thanks to Julius Werner for the suggested fix.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/41988/2

-- To view, visit https://review.coreboot.org/c/coreboot/+/41988 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iba341a750cce8334da4dcf6353ca8cd1268d170f Gerrit-Change-Number: 41988 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: newpatchset

Julius Werner (Code Review)

3:15 a.m.

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41988 )

Change subject: chromeos/cr50_enable_update.c: Modify recovery flow for cr50 ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41988/2/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/cr50_enable_update.c:

https://review.coreboot.org/c/coreboot/+/41988/2/src/vendorcode/google/chrom... PS2, Line 77: * spooled by recovery_reason := VB2_RECOVERY_TRAIN_AND_REBOOT. nit: This still doesn't really explain why it's checking for FORCE_RECOVERY. How about

/* Never update during manually-triggered recovery to ensure update cannot interfere. Non-manual VB2_RECOVERY_TRAIN_AND_REBOOT sometimes used to update in factory. */

-- To view, visit https://review.coreboot.org/c/coreboot/+/41988 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iba341a750cce8334da4dcf6353ca8cd1268d170f Gerrit-Change-Number: 41988 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 03 Jun 2020 01:15:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

4:10 p.m.

Hello Sam McNally, build bot (Jenkins), Furquan Shaikh, Julius Werner,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41988

to look at the new patch set (#3).

Change subject: chromeos/cr50_enable_update.c: Modify recovery flow for cr50 ......................................................................

chromeos/cr50_enable_update.c: Modify recovery flow for cr50

Leave manual recovery unaffected so at least that would still work even if Cr50 wedges in a weird way that it thinks it has an update on every boot or something.

Setting the recovery_reason to VB2_RECOVERY_TRAIN_AND_REBOOT allows the update to be applied.

BUG=b:154071064 BRANCH=none TEST=builds

Thanks to Julius Werner for the suggested fix.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/41988/3

-- To view, visit https://review.coreboot.org/c/coreboot/+/41988 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iba341a750cce8334da4dcf6353ca8cd1268d170f Gerrit-Change-Number: 41988 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Edward O'Callaghan (Code Review)

4:11 p.m.

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41988 )

Change subject: chromeos/cr50_enable_update.c: Modify recovery flow for cr50 ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41988/2/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/cr50_enable_update.c:

https://review.coreboot.org/c/coreboot/+/41988/2/src/vendorcode/google/chrom... PS2, Line 77: * spooled by recovery_reason := VB2_RECOVERY_TRAIN_AND_REBOOT.

...

nit: This still doesn't really explain why it's checking for FORCE_RECOVERY. How about […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/41988 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iba341a750cce8334da4dcf6353ca8cd1268d170f Gerrit-Change-Number: 41988 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 03 Jun 2020 14:11:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner jwerner@chromium.org Gerrit-MessageType: comment

Julius Werner (Code Review)

11:43 p.m.

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41988 )

Change subject: chromeos/cr50_enable_update.c: Modify recovery flow for cr50 ......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/41988/3/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/cr50_enable_update.c:

https://review.coreboot.org/c/coreboot/+/41988/3/src/vendorcode/google/chrom... PS3, Line 75: * nit: There should be only one asterisk here, not two.

-- To view, visit https://review.coreboot.org/c/coreboot/+/41988 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iba341a750cce8334da4dcf6353ca8cd1268d170f Gerrit-Change-Number: 41988 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 03 Jun 2020 21:43:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Georgi (Code Review)

6 Jun 6 Jun

11:39 a.m.

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41988 )

Change subject: chromeos/cr50_enable_update.c: Modify recovery flow for cr50 ......................................................................

chromeos/cr50_enable_update.c: Modify recovery flow for cr50

Leave manual recovery unaffected so at least that would still work even if Cr50 wedges in a weird way that it thinks it has an update on every boot or something.

Setting the recovery_reason to VB2_RECOVERY_TRAIN_AND_REBOOT allows the update to be applied.

BUG=b:154071064 BRANCH=none TEST=builds

Thanks to Julius Werner for the suggested fix.

Change-Id: Iba341a750cce8334da4dcf6353ca8cd1268d170f Signed-off-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/41988 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/vendorcode/google/chromeos/cr50_enable_update.c 1 file changed, 6 insertions(+), 2 deletions(-)

Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved

diff --git a/src/vendorcode/google/chromeos/cr50_enable_update.c b/src/vendorcode/google/chromeos/cr50_enable_update.c index 5b3a1a3..e30fe2a 100644 --- a/src/vendorcode/google/chromeos/cr50_enable_update.c +++ b/src/vendorcode/google/chromeos/cr50_enable_update.c @@ -72,8 +72,12 @@ int cr50_reset_reqd = 0; uint8_t num_restored_headers;

- /* Nothing to do on recovery mode. */ - if (vboot_recovery_mode_enabled()) + /** + * Never update during manually-triggered recovery to ensure update + * cannot interfere. Non-manual VB2_RECOVERY_TRAIN_AND_REBOOT + * sometimes used to update in factory. + */ + if (vboot_get_context()->flags & VB2_CONTEXT_FORCE_RECOVERY_MODE) return;

ret = tlcl_lib_init();

-- To view, visit https://review.coreboot.org/c/coreboot/+/41988 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iba341a750cce8334da4dcf6353ca8cd1268d170f Gerrit-Change-Number: 41988 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1431

days inactive

1435

days old

coreboot-gerrit@coreboot.org

9 comments

4 participants

tags (0)

participants (4)

Edward O'Callaghan (Code Review)
Furquan Shaikh (Code Review)
Julius Werner (Code Review)
Patrick Georgi (Code Review)