Re: [PATCH v4 0/5] kexec_file: Add buffer hand-over for the next kernel

[Mimi Zohar]

On Thu, 2016-09-08 at 23:07 -0500, Eric W. Biederman wrote:
> Thiago Jung Bauermann <bauerman@xxxxxxxxxxxxxxxxxx> writes:
> 
> > Am Mittwoch, 07 September 2016, 09:19:40 schrieb Eric W. Biederman:
> >> ebiederm@xxxxxxxxxxxx (Eric W. Biederman) writes:
> >> > Thiago Jung Bauermann <bauerman@xxxxxxxxxxxxxxxxxx> writes:
> >> >> Hello,
> >> >> 
> >> >> The purpose of this new version of the series is to fix a small issue
> >> >> that I found, which is that the kernel doesn't remove the memory
> >> >> reservation for the hand-over buffer it received from the previous
> >> >> kernel in the device tree it sets up for the next kernel. The result
> >> >> is that for each successive kexec, a stale hand-over buffer is left
> >> >> behind, wasting memory.
> >> >> 
> >> >> This is fixed by changes to kexec_free_handover_buffer and
> >> >> setup_handover_buffer in patch 2. The other change is to fix checkpatch
> >> >> warnings in the last patch.
> >> > 
> >> > This is fundamentally broken.  You do not increase the integrity of a
> >> > system by dropping integrity checks.
> >> > 
> >> > No. No. No. No.
> >> > 
> >> > Nacked-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> >
> > The IMA measurement list can be verified without the need of a checksum over 
> > its contents by replaying the PCR extend operations and checking that the 
> > result matches the registers in the TPM device. So the fact that it is not 
> > part of the kexec segments checksum verification doesn't actually reduce the 
> > integrity of the system.
> 
> Bit flips and errant DMA transfers are the concern here.  That happens
> routinely and can easily result in a corrupt data structure which may be
> non-trivial to verify.

Point taken.  Before using the measurement list, the hash needs to be
verified.  So then the question is when and where that hash should be
taken and included.

New measurements can take place between the kexec load and execute.
Those measurements would be included in an additional execute of kexec
load, as you suggested. There are additional new measurements that are
the result of the kexec execute itself, as Thiago pointed out.  Having
to know a priori what those measurements are, or should be, makes
carrying the measurement list across kexec brittle.

One solution, as suggested by Thiago, would be to calculate the
measurement list hash, include the hash in the ima_kexec_hdr and verify
the hash, before restoring the measurement list.  Another option would
be to calculate the image hash on kexec load either with  a zero filled
measurement list segment or without the segment, verify the image hash
on kexec execute, before re-calculating the image hash.

> > Currently, IMA doesn't perform that verification when it restores the 
> > measurement list from the kexec handover buffer, so if you believe it's 
> > necessary to do that check at boot time, we could do one of the following:
> >
> > 1. Have IMA replay the PCR extend operations when it restores the 
> > measurement list from the handover buffer and validate it against the TPM 
> > PCRs, or
> >
> > 2. Have a buffer hash in the ima_kexec_hdr that IMA includes in the handover 
> > buffer, and verify the buffer checksum before restoring the measurement 
> > list.
> >
> > What do you think?
> 
> I think you are playing very much with fire and I am extremely
> uncomfortable with the entire concept.  I think you are making things
> more complicated in a way that will allow system to try and start
> booting when their memory is correct.  Which may wind up corrupting
> someones non-volatile storage.

Everything that was previously included in the image hash, before this
proposed patch set, is still included in the image hash.

Lets assume  for a moment that the measurement list hash is stored in
the ima_kexec_hdr.  In addition to preventing the measurement list from
being restored, we could either allow the reboot without the measurement
list to occur or halt the system.

Rebooting without the measurement list would result in the attestation
of the system failing.

> It makes me doubly nervous that this adds a general purpose facility
> that is generally not at all reusable.

Thiago has already agreed to reverting the skip hash bit. 

> I have seen malicious actors cause entirely too much damage to be at all
> comfortable using a data structure before we validate that it was valid
> before we started booting.  This isn't the same case but it is close
> enough I don't trust someone just splatting data structures.
> We can't guarantee integrity but we should not bypass best practices
> either.
> 
> >> To be constructive the way we have handled similiar situations in the
> >> past (hotplu memory) is to call kexec_load again.
> >
> > Thanks for your suggestion. Unfortunately it's always possible for new 
> > measurements to be added to the measurement list between the kexec_file_load 
> > and the reboot. We see that happen in practice with system scripts and 
> > configuration files that are only read or executed during the reboot 
> > process. They are only measured by IMA as a result of the kexec execute.
> 
> If I understand what you are saying correctly I expect things could be
> setup so that those measurements are forced to happen before kexec load.

> Especially in a boot loader context which you described earlier I
> believe you should have quite a lot of control of the system.  Having a
> facility that fundamentally undermines the design of kexec for a case
> where someone might do something you have not predicted does not make me
> comfortable.
> 
> Which is to say I don't see why you can't measure things before the
> kexec_load system call in a tightly controlled setup like a boot loader.
> Which should make it that in practice no measurements change.  I believe
> that should increase the reliability of the system overall.

Either we measure everything possible, making the measurement list
unnecessarily large, or have a brittle system that could break the
attestation of the target system, if any packages add new files/scripts
that are accessed on a soft reboot.

> Having code in the kernel that updates a buffer that kexec will use
> after that buffer is loaded in memory honestely gives me the heebie
> jeebies.

To address your understandable concerns, we suggested two possible
solutions for including and verifying the measurement list hash.
- calculate the image hash on load, verify it on kexec reload, before
recalculating the image hash with the measurement list segment.
- defer hashing and verifying the measurement list hash to IMA.

Mimi