Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6871044:
Do not advance kernel version in TPM if we are in firmware B trying a new firmware (Closed)
|
Created:
9 years, 8 months ago by Randall Spangler Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org Visibility:
Public. |
DescriptionDo not advance kernel version in TPM if we are in firmware B trying a new firmware
Change-Id: If5b6390d011d743689cf96e49202358397663651
R=bleung@chromium.org,dlaurie@chromium.org,sumit@chromium.org
BUG=chrome-os-partner:3367
TEST=make && make runtests
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=700fc49
Patch Set 1 #
Total comments: 1
Patch Set 2 : Don't advance kernel version in developer firmware #
Total comments: 3
Messages
Total messages: 17 (0 generated)
http://codereview.chromium.org/6871044/diff/1/firmware/lib/vboot_kernel.c File firmware/lib/vboot_kernel.c (right): http://codereview.chromium.org/6871044/diff/1/firmware/lib/vboot_kernel.c#new... firmware/lib/vboot_kernel.c:600: if ((kBootRecovery != boot_mode) && Do we need to set TPM in developer firmware? I agree about normal mode and developer mode with normal firmware but not sure about developer firmware.
Good point. We added the developer check to protect root shell users back when developer firmware was the only way to get them a root shell, before Will's fix. Now that root shell developers use normal firmware, we should only update the version if firmware is normal. In that case we can simplify the check, because normal firmware finds a good partition only if the key block is valid. (PTAL)
LGTM This seems reasonable to be. There might be other places where we are using dev mode in this file. I am assuming those are fine but may be good to have a quick look. Also, might be good to get someone else also review the code.
On 2011/04/17 17:35:06, Sumit wrote: > LGTM > > This seems reasonable to be. There might be other places where we are using dev > mode in this file. I am assuming those are fine but may be good to have a quick > look. > > Also, might be good to get someone else also review the code. It looks good to me, but I haven't done much in vboot reference. Adding gaurav to the list of reviewers.
LGTM The other uses of dev mode look ok to me.
Drive-by LGTM with some questions for future clarity. http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c File firmware/lib/vboot_kernel.c (right): http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... firmware/lib/vboot_kernel.c:601: !((1 == shared->firmware_index) && (shared->flags & VBSD_FWB_TRIED))) { What case will shared->firmware_index==1 without VBSD_FWB_TRIED? Do we need to worry about that case too? (E.g., corrupted FWA). Or is this only when it appears an AU triggered the fw volume use? http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... firmware/lib/vboot_kernel.c:607: if ((lowest_version > tpm_version) && Since we only increment when the lowest_version of both kernels is > tpm_version, why would we ever trigger a bug on a kernel version update if in VSD_FWB_TRIED? This seems more relevant for a firmware version change, but maybe I'm missing something? Is this just in case there is a bug in the new firmware that doesn't respect the existing lower-versioning kernels? If so, please spell that out in the comments! It will likely answer all my questions :)
On further offline discussion, it makes sense that a new firmware volume may have a different key and be unable to validate the older kernel volumes that are valid with the other firmware. It'd be good to spell that out even if it is obvious when you have more context :) On 2011/04/17 17:53:18, Will Drewry wrote: > Drive-by LGTM with some questions for future clarity. > > http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c > File firmware/lib/vboot_kernel.c (right): > > http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... > firmware/lib/vboot_kernel.c:601: !((1 == shared->firmware_index) && > (shared->flags & VBSD_FWB_TRIED))) { > What case will shared->firmware_index==1 without VBSD_FWB_TRIED? Do we need to > worry about that case too? (E.g., corrupted FWA). Or is this only when it > appears an AU triggered the fw volume use? > > http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... > firmware/lib/vboot_kernel.c:607: if ((lowest_version > tpm_version) && > Since we only increment when the lowest_version of both kernels is > > tpm_version, why would we ever trigger a bug on a kernel version update if in > VSD_FWB_TRIED? This seems more relevant for a firmware version change, but > maybe I'm missing something? > > Is this just in case there is a bug in the new firmware that doesn't respect the > existing lower-versioning kernels? > > If so, please spell that out in the comments! It will likely answer all my > questions :)
http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c File firmware/lib/vboot_kernel.c (right): http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... firmware/lib/vboot_kernel.c:607: if ((lowest_version > tpm_version) && Belated comment: I'd really like to see a check for the following invariant: lowest_version is not valid until it has been compared to both kernel versions. It is not written until it is valid. Is there any situation where the invariant won't be true? This should, hopefully, catch other code paths where we can run into bad situations.
On Sun, Apr 17, 2011 at 12:32 PM, <gauravsh@chromium.org> wrote: > > > http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c > File firmware/lib/vboot_kernel.c (right): > > > http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... > firmware/lib/vboot_kernel.c:607: if ((lowest_version > tpm_version) && > Belated comment: I'd really like to see a check for the following > invariant: lowest_version is not valid until it has been compared to > both kernel versions. It is not written until it is valid. > > +1 on this > Is there any situation where the invariant won't be true? > > This should, hopefully, catch other code paths where we can run into bad > situations. > > > http://codereview.chromium.org/6871044/ >
On Sun, Apr 17, 2011 at 12:49 PM, Sumit Gwalani <sumitg@google.com> wrote: > > > On Sun, Apr 17, 2011 at 12:32 PM, <gauravsh@chromium.org> wrote: > >> >> >> http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c >> File firmware/lib/vboot_kernel.c (right): >> >> >> http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... >> firmware/lib/vboot_kernel.c:607: if ((lowest_version > tpm_version) && >> Belated comment: I'd really like to see a check for the following >> invariant: lowest_version is not valid until it has been compared to >> both kernel versions. It is not written until it is valid. >> >> > +1 on this > By "both" do you mean "/dev/sda2 and /dev/sda4"? Or "all kernels"? That is, does the firmware now need specific knowledge that /dev/sda2 and /dev/sda4 are special? Currently, it treats all kernels equally. Thanks, Randall > > Is there any situation where the invariant won't be true? >> >> This should, hopefully, catch other code paths where we can run into bad >> situations. >> >> >> http://codereview.chromium.org/6871044/ >> > >
On 2011/04/17 19:53:01, Randall wrote: > On Sun, Apr 17, 2011 at 12:49 PM, Sumit Gwalani <mailto:sumitg@google.com> wrote: > > > > > > > On Sun, Apr 17, 2011 at 12:32 PM, <mailto:gauravsh@chromium.org> wrote: > > > >> > >> > >> http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c > >> File firmware/lib/vboot_kernel.c (right): > >> > >> > >> > http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... > >> firmware/lib/vboot_kernel.c:607: if ((lowest_version > tpm_version) && > >> Belated comment: I'd really like to see a check for the following > >> invariant: lowest_version is not valid until it has been compared to > >> both kernel versions. It is not written until it is valid. > >> > >> > > +1 on this > > > > By "both" do you mean "/dev/sda2 and /dev/sda4"? Or "all kernels"? All kernels, of which we only have 2 right now. Basically, my point is that unless there is a good reason to do so, we should have a sanity check just before rollback versions are committed that ensures that lowest_versions really captures the state of all the kernels irrespective of whether or not they failed other checks or are invalid for other reasons. Of course, we will only ever commit a version for a kernel that actually verifies. The existing checks already do that. If there are some other considerations or special cases due to which this will not work or is an invalid heuristic, we can discuss it offline. My worry is that the state matrix with tries and other flags, and the combination of multiple firmwares and kernels is complex. So it is rather tricky to get all the code paths exactly right - adding sanity checking heuristics, however explicit, to avoid transitioning to a bad states would be useful. Things like - don't commit version changes if we are in dev mode (which you added), are examples of one such heuristic. > > That is, does the firmware now need specific knowledge that /dev/sda2 and > /dev/sda4 are special? Currently, it treats all kernels equally. > > Thanks, > Randall > > > > > > Is there any situation where the invariant won't be true? > >> > >> This should, hopefully, catch other code paths where we can run into bad > >> situations. > >> > >> > >> http://codereview.chromium.org/6871044/ > >> > > > >
On Sun, Apr 17, 2011 at 3:15 PM, <gauravsh@chromium.org> wrote: > On 2011/04/17 19:53:01, Randall wrote: > >> On Sun, Apr 17, 2011 at 12:49 PM, Sumit Gwalani <mailto:sumitg@google.com >> > >> > wrote: > > > > >> > >> > On Sun, Apr 17, 2011 at 12:32 PM, <mailto:gauravsh@chromium.org> wrote: >> > >> >> >> >> >> >> >> > > http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c > >> >> File firmware/lib/vboot_kernel.c (right): >> >> >> >> >> >> >> > > > http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... > >> >> firmware/lib/vboot_kernel.c:607: if ((lowest_version > tpm_version) && >> >> Belated comment: I'd really like to see a check for the following >> >> invariant: lowest_version is not valid until it has been compared to >> >> both kernel versions. It is not written until it is valid. >> >> >> >> >> > +1 on this >> > >> > > By "both" do you mean "/dev/sda2 and /dev/sda4"? Or "all kernels"? >> > > All kernels, of which we only have 2 right now. Basically, my point is that > unless there is a good reason to do so, we should have a sanity check just > before rollback versions are committed that ensures that lowest_versions > really > captures the state of all the kernels irrespective of whether or not they > failed > other checks or are invalid for other reasons. > Doesn't that mean if an attacker puts a kernel preamble in /dev/sda6 with a key value of 0x00010001, we'll never advance the kernel version in the TPM? > > Of course, we will only ever commit a version for a kernel that actually > verifies. The existing checks already do that. > > If there are some other considerations or special cases due to which this > will > not work or is an invalid heuristic, we can discuss it offline. > > My worry is that the state matrix with tries and other flags, and the > combination of multiple firmwares and kernels is complex. So it is rather > tricky > to get all the code paths exactly right - adding sanity checking > heuristics, > however explicit, to avoid transitioning to a bad states would be useful. > Things > like - don't commit version changes if we are in dev mode (which you > added), are > examples of one such heuristic. > > > > That is, does the firmware now need specific knowledge that /dev/sda2 and >> /dev/sda4 are special? Currently, it treats all kernels equally. >> > > Thanks, >> Randall >> > > > > >> > Is there any situation where the invariant won't be true? >> >> >> >> This should, hopefully, catch other code paths where we can run into >> bad >> >> situations. >> >> >> >> >> >> http://codereview.chromium.org/6871044/ >> >> >> > >> > >> > > > > http://codereview.chromium.org/6871044/ >
On Sun, Apr 17, 2011 at 5:03 PM, Randall Spangler <rspangler@google.com>wrote: > > > On Sun, Apr 17, 2011 at 3:15 PM, <gauravsh@chromium.org> wrote: > >> On 2011/04/17 19:53:01, Randall wrote: >> >>> On Sun, Apr 17, 2011 at 12:49 PM, Sumit Gwalani <mailto: >>> sumitg@google.com> >>> >> wrote: >> >> >> > >>> > >>> > On Sun, Apr 17, 2011 at 12:32 PM, <mailto:gauravsh@chromium.org> >>> wrote: >>> > >>> >> >>> >> >>> >> >>> >> >> http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c >> >>> >> File firmware/lib/vboot_kernel.c (right): >>> >> >>> >> >>> >> >>> >> >> >> http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... >> >>> >> firmware/lib/vboot_kernel.c:607: if ((lowest_version > tpm_version) && >>> >> Belated comment: I'd really like to see a check for the following >>> >> invariant: lowest_version is not valid until it has been compared to >>> >> both kernel versions. It is not written until it is valid. >>> >> >>> >> >>> > +1 on this >>> > >>> >> >> By "both" do you mean "/dev/sda2 and /dev/sda4"? Or "all kernels"? >>> >> >> All kernels, of which we only have 2 right now. Basically, my point is >> that >> unless there is a good reason to do so, we should have a sanity check just >> before rollback versions are committed that ensures that lowest_versions >> really >> captures the state of all the kernels irrespective of whether or not they >> failed >> other checks or are invalid for other reasons. >> > > Doesn't that mean if an attacker puts a kernel preamble in /dev/sda6 with a > key value of 0x00010001, we'll never advance the kernel version in the TPM? > I guess it depends on what is the 3rd kernel meant for? Is it ever meant to contain an "official" kernel to boot from? If not, then we don't care what the 3rd kernel says and it is ignored. The AU client is meant to overwrite the old copy of the kernel once we are sure that the new kernel is good and bootable. TPM finalization should not happen until we have reached that stable state. > > >> >> Of course, we will only ever commit a version for a kernel that actually >> verifies. The existing checks already do that. >> >> If there are some other considerations or special cases due to which this >> will >> not work or is an invalid heuristic, we can discuss it offline. >> >> My worry is that the state matrix with tries and other flags, and the >> combination of multiple firmwares and kernels is complex. So it is rather >> tricky >> to get all the code paths exactly right - adding sanity checking >> heuristics, >> however explicit, to avoid transitioning to a bad states would be useful. >> Things >> like - don't commit version changes if we are in dev mode (which you >> added), are >> examples of one such heuristic. >> >> >> >> That is, does the firmware now need specific knowledge that /dev/sda2 and >>> /dev/sda4 are special? Currently, it treats all kernels equally. >>> >> >> Thanks, >>> Randall >>> >> >> >> > >>> > Is there any situation where the invariant won't be true? >>> >> >>> >> This should, hopefully, catch other code paths where we can run into >>> bad >>> >> situations. >>> >> >>> >> >>> >> http://codereview.chromium.org/6871044/ >>> >> >>> > >>> > >>> >> >> >> >> http://codereview.chromium.org/6871044/ >> > > -- -g
On Sun, Apr 17, 2011 at 5:19 PM, Gaurav Shah <gauravsh@chromium.org> wrote: > On Sun, Apr 17, 2011 at 5:03 PM, Randall Spangler <rspangler@google.com>wrote: > >> >> >> On Sun, Apr 17, 2011 at 3:15 PM, <gauravsh@chromium.org> wrote: >> >>> On 2011/04/17 19:53:01, Randall wrote: >>> >>>> On Sun, Apr 17, 2011 at 12:49 PM, Sumit Gwalani <mailto: >>>> sumitg@google.com> >>>> >>> wrote: >>> >>> >>> > >>>> > >>>> > On Sun, Apr 17, 2011 at 12:32 PM, <mailto:gauravsh@chromium.org> >>>> wrote: >>>> > >>>> >> >>>> >> >>>> >> >>>> >>> >>> http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c >>> >>>> >> File firmware/lib/vboot_kernel.c (right): >>>> >> >>>> >> >>>> >> >>>> >>> >>> >>> http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... >>> >>>> >> firmware/lib/vboot_kernel.c:607: if ((lowest_version > tpm_version) >>>> && >>>> >> Belated comment: I'd really like to see a check for the following >>>> >> invariant: lowest_version is not valid until it has been compared to >>>> >> both kernel versions. It is not written until it is valid. >>>> >> >>>> >> >>>> > +1 on this >>>> > >>>> >>> >>> By "both" do you mean "/dev/sda2 and /dev/sda4"? Or "all kernels"? >>>> >>> >>> All kernels, of which we only have 2 right now. Basically, my point is >>> that >>> unless there is a good reason to do so, we should have a sanity check >>> just >>> before rollback versions are committed that ensures that lowest_versions >>> really >>> captures the state of all the kernels irrespective of whether or not they >>> failed >>> other checks or are invalid for other reasons. >>> >> >> Doesn't that mean if an attacker puts a kernel preamble in /dev/sda6 with >> a key value of 0x00010001, we'll never advance the kernel version in the >> TPM? >> > > I guess it depends on what is the 3rd kernel meant for? Is it ever meant to > contain an "official" kernel to boot from? If not, then we don't care what > the 3rd kernel says and it is ignored. > That was why I'd asked if we're making /dev/sda2 and /dev/sda4 special to the firmware. We'd need to add checks to see if we've seen good kernels in SSD partitions 2 and 4. If no, don't update the TPM. The firmware could still boot from any partition, but would never update the TPM unless partitions 2 and 4 are both valid AND bootable (that is, they have a valid preamble signed with the current kernel subkey, non-zero priority, and either (tries>0 or success=1). It's doable; it just means that SSD partition layout would be locked by compatibility with the firmware. If we later want to change the partition layout - for example, to reorder the partitions, or to add a 3rd kernel as a backup for the current kernel during AU - we'd need to rev the firmware. - Randall > The AU client is meant to overwrite the old copy of the kernel once we are > sure that the new kernel is good and bootable. TPM finalization should not > happen until we have reached that stable state. > > >> >> >>> >>> Of course, we will only ever commit a version for a kernel that actually >>> verifies. The existing checks already do that. >>> >>> If there are some other considerations or special cases due to which this >>> will >>> not work or is an invalid heuristic, we can discuss it offline. >>> >>> My worry is that the state matrix with tries and other flags, and the >>> combination of multiple firmwares and kernels is complex. So it is rather >>> tricky >>> to get all the code paths exactly right - adding sanity checking >>> heuristics, >>> however explicit, to avoid transitioning to a bad states would be useful. >>> Things >>> like - don't commit version changes if we are in dev mode (which you >>> added), are >>> examples of one such heuristic. >>> >>> >>> >>> That is, does the firmware now need specific knowledge that /dev/sda2 >>>> and >>>> /dev/sda4 are special? Currently, it treats all kernels equally. >>>> >>> >>> Thanks, >>>> Randall >>>> >>> >>> >>> > >>>> > Is there any situation where the invariant won't be true? >>>> >> >>>> >> This should, hopefully, catch other code paths where we can run into >>>> bad >>>> >> situations. >>>> >> >>>> >> >>>> >> http://codereview.chromium.org/6871044/ >>>> >> >>>> > >>>> > >>>> >>> >>> >>> >>> http://codereview.chromium.org/6871044/ >>> >> >> > > > -- > -g >
On Sun, Apr 17, 2011 at 5:59 PM, Randall Spangler <rspangler@google.com>wrote: > > > On Sun, Apr 17, 2011 at 5:19 PM, Gaurav Shah <gauravsh@chromium.org>wrote: > >> On Sun, Apr 17, 2011 at 5:03 PM, Randall Spangler <rspangler@google.com>wrote: >> >>> >>> >>> On Sun, Apr 17, 2011 at 3:15 PM, <gauravsh@chromium.org> wrote: >>> >>>> On 2011/04/17 19:53:01, Randall wrote: >>>> >>>>> On Sun, Apr 17, 2011 at 12:49 PM, Sumit Gwalani <mailto: >>>>> sumitg@google.com> >>>>> >>>> wrote: >>>> >>>> >>>> > >>>>> > >>>>> > On Sun, Apr 17, 2011 at 12:32 PM, <mailto:gauravsh@chromium.org> >>>>> wrote: >>>>> > >>>>> >> >>>>> >> >>>>> >> >>>>> >>>> >>>> http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c >>>> >>>>> >> File firmware/lib/vboot_kernel.c (right): >>>>> >> >>>>> >> >>>>> >> >>>>> >>>> >>>> >>>> http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... >>>> >>>>> >> firmware/lib/vboot_kernel.c:607: if ((lowest_version > tpm_version) >>>>> && >>>>> >> Belated comment: I'd really like to see a check for the following >>>>> >> invariant: lowest_version is not valid until it has been compared to >>>>> >> both kernel versions. It is not written until it is valid. >>>>> >> >>>>> >> >>>>> > +1 on this >>>>> > >>>>> >>>> >>>> By "both" do you mean "/dev/sda2 and /dev/sda4"? Or "all kernels"? >>>>> >>>> >>>> All kernels, of which we only have 2 right now. Basically, my point is >>>> that >>>> unless there is a good reason to do so, we should have a sanity check >>>> just >>>> before rollback versions are committed that ensures that lowest_versions >>>> really >>>> captures the state of all the kernels irrespective of whether or not >>>> they failed >>>> other checks or are invalid for other reasons. >>>> >>> >>> Doesn't that mean if an attacker puts a kernel preamble in /dev/sda6 with >>> a key value of 0x00010001, we'll never advance the kernel version in the >>> TPM? >>> >> >> I guess it depends on what is the 3rd kernel meant for? Is it ever meant >> to contain an "official" kernel to boot from? If not, then we don't care >> what the 3rd kernel says and it is ignored. >> > > That was why I'd asked if we're making /dev/sda2 and /dev/sda4 special to > the firmware. We'd need to add checks to see if we've seen good kernels in > SSD partitions 2 and 4. If no, don't update the TPM. > > The firmware could still boot from any partition, but would never update > the TPM unless partitions 2 and 4 are both valid AND bootable (that is, they > have a valid preamble signed with the current kernel subkey, non-zero > priority, and either (tries>0 or success=1). > > It's doable; it just means that SSD partition layout would be locked by > compatibility with the firmware. If we later want to change the partition > layout - for example, to reorder the partitions, or to add a 3rd kernel as a > backup for the current kernel during AU - we'd need to rev the firmware. > That is reasonable. We need a firmware update for kernel key changes anyway. So a firmware update for supporting the 3rd kernel partition in the future doesn't seem like that much of big deal. > - Randall > > >> The AU client is meant to overwrite the old copy of the kernel once we are >> sure that the new kernel is good and bootable. TPM finalization should not >> happen until we have reached that stable state. >> >> >>> >>> >>>> >>>> Of course, we will only ever commit a version for a kernel that actually >>>> verifies. The existing checks already do that. >>>> >>>> If there are some other considerations or special cases due to which >>>> this will >>>> not work or is an invalid heuristic, we can discuss it offline. >>>> >>>> My worry is that the state matrix with tries and other flags, and the >>>> combination of multiple firmwares and kernels is complex. So it is >>>> rather tricky >>>> to get all the code paths exactly right - adding sanity checking >>>> heuristics, >>>> however explicit, to avoid transitioning to a bad states would be >>>> useful. Things >>>> like - don't commit version changes if we are in dev mode (which you >>>> added), are >>>> examples of one such heuristic. >>>> >>>> >>>> >>>> That is, does the firmware now need specific knowledge that /dev/sda2 >>>>> and >>>>> /dev/sda4 are special? Currently, it treats all kernels equally. >>>>> >>>> >>>> Thanks, >>>>> Randall >>>>> >>>> >>>> >>>> > >>>>> > Is there any situation where the invariant won't be true? >>>>> >> >>>>> >> This should, hopefully, catch other code paths where we can run into >>>>> bad >>>>> >> situations. >>>>> >> >>>>> >> >>>>> >> http://codereview.chromium.org/6871044/ >>>>> >> >>>>> > >>>>> > >>>>> >>>> >>>> >>>> >>>> http://codereview.chromium.org/6871044/ >>>> >>> >>> >> >> >> -- >> -g >> > > -- -g
Why do we have a third kernel partition anyway? Seems like there's some risk in having something like that which isn't really used in normal operation from a security standpoint. Sonny On Sun, Apr 17, 2011 at 10:20 PM, Gaurav Shah <gauravsh@chromium.org> wrote: > On Sun, Apr 17, 2011 at 5:59 PM, Randall Spangler <rspangler@google.com> > wrote: >> >> >> On Sun, Apr 17, 2011 at 5:19 PM, Gaurav Shah <gauravsh@chromium.org> >> wrote: >>> >>> On Sun, Apr 17, 2011 at 5:03 PM, Randall Spangler <rspangler@google.com> >>> wrote: >>>> >>>> >>>> On Sun, Apr 17, 2011 at 3:15 PM, <gauravsh@chromium.org> wrote: >>>>> >>>>> On 2011/04/17 19:53:01, Randall wrote: >>>>>> >>>>>> On Sun, Apr 17, 2011 at 12:49 PM, Sumit Gwalani >>>>>> <mailto:sumitg@google.com> >>>>> >>>>> wrote: >>>>> >>>>>> > >>>>>> > >>>>>> > On Sun, Apr 17, 2011 at 12:32 PM, <mailto:gauravsh@chromium.org> >>>>>> > wrote: >>>>>> > >>>>>> >> >>>>>> >> >>>>>> >> >>>>> >>>>> >>>>> http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c >>>>>> >>>>>> >> File firmware/lib/vboot_kernel.c (right): >>>>>> >> >>>>>> >> >>>>>> >> >>>>> >>>>> >>>>> http://codereview.chromium.org/6871044/diff/3002/firmware/lib/vboot_kernel.c#... >>>>>> >>>>>> >> firmware/lib/vboot_kernel.c:607: if ((lowest_version > tpm_version) >>>>>> >> && >>>>>> >> Belated comment: I'd really like to see a check for the following >>>>>> >> invariant: lowest_version is not valid until it has been compared >>>>>> >> to >>>>>> >> both kernel versions. It is not written until it is valid. >>>>>> >> >>>>>> >> >>>>>> > +1 on this >>>>>> > >>>>> >>>>>> By "both" do you mean "/dev/sda2 and /dev/sda4"? Or "all kernels"? >>>>> >>>>> All kernels, of which we only have 2 right now. Basically, my point is >>>>> that >>>>> unless there is a good reason to do so, we should have a sanity check >>>>> just >>>>> before rollback versions are committed that ensures that >>>>> lowest_versions really >>>>> captures the state of all the kernels irrespective of whether or not >>>>> they failed >>>>> other checks or are invalid for other reasons. >>>> >>>> Doesn't that mean if an attacker puts a kernel preamble in /dev/sda6 >>>> with a key value of 0x00010001, we'll never advance the kernel version in >>>> the TPM? >>> >>> I guess it depends on what is the 3rd kernel meant for? Is it ever meant >>> to contain an "official" kernel to boot from? If not, then we don't care >>> what the 3rd kernel says and it is ignored. >> >> That was why I'd asked if we're making /dev/sda2 and /dev/sda4 special to >> the firmware. We'd need to add checks to see if we've seen good kernels in >> SSD partitions 2 and 4. If no, don't update the TPM. >> The firmware could still boot from any partition, but would never update >> the TPM unless partitions 2 and 4 are both valid AND bootable (that is, they >> have a valid preamble signed with the current kernel subkey, non-zero >> priority, and either (tries>0 or success=1). >> It's doable; it just means that SSD partition layout would be locked by >> compatibility with the firmware. If we later want to change the partition >> layout - for example, to reorder the partitions, or to add a 3rd kernel as a >> backup for the current kernel during AU - we'd need to rev the firmware. > > That is reasonable. We need a firmware update for kernel key changes anyway. > So a firmware update for supporting the 3rd kernel partition in the future > doesn't seem like that much of big deal. >> >> - Randall >> >>> >>> The AU client is meant to overwrite the old copy of the kernel once we >>> are sure that the new kernel is good and bootable. TPM finalization should >>> not happen until we have reached that stable state. >>> >>>> >>>> >>>>> >>>>> Of course, we will only ever commit a version for a kernel that >>>>> actually >>>>> verifies. The existing checks already do that. >>>>> >>>>> If there are some other considerations or special cases due to which >>>>> this will >>>>> not work or is an invalid heuristic, we can discuss it offline. >>>>> >>>>> My worry is that the state matrix with tries and other flags, and the >>>>> combination of multiple firmwares and kernels is complex. So it is >>>>> rather tricky >>>>> to get all the code paths exactly right - adding sanity checking >>>>> heuristics, >>>>> however explicit, to avoid transitioning to a bad states would be >>>>> useful. Things >>>>> like - don't commit version changes if we are in dev mode (which you >>>>> added), are >>>>> examples of one such heuristic. >>>>> >>>>> >>>>>> That is, does the firmware now need specific knowledge that /dev/sda2 >>>>>> and >>>>>> /dev/sda4 are special? Currently, it treats all kernels equally. >>>>> >>>>>> Thanks, >>>>>> Randall >>>>> >>>>> >>>>>> > >>>>>> > Is there any situation where the invariant won't be true? >>>>>> >> >>>>>> >> This should, hopefully, catch other code paths where we can run >>>>>> >> into bad >>>>>> >> situations. >>>>>> >> >>>>>> >> >>>>>> >> http://codereview.chromium.org/6871044/ >>>>>> >> >>>>>> > >>>>>> > >>>>> >>>>> >>>>> >>>>> http://codereview.chromium.org/6871044/ >>>> >>> >>> >>> >>> -- >>> -g >> > > > > -- > -g > |
