Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Mimi Zohar]

Hi Petko,

Thank you for review!

On Fri, 2016-08-05 at 11:44 +0300, Petko Manolov wrote:
> On 16-08-04 08:24:29, Mimi Zohar wrote:
> > The TPM PCRs are only reset on a hard reboot.  In order to validate a
> > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> > of the running kernel must be saved and restored on boot.  This patch
> > restores the measurement list.
> > 
> > Changelog:
> > - call ima_load_kexec_buffer() (Thiago)
> > 
> > Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx>
> > ---
> >  security/integrity/ima/Makefile       |   1 +
> >  security/integrity/ima/ima.h          |  10 ++
> >  security/integrity/ima/ima_init.c     |   2 +
> >  security/integrity/ima/ima_kexec.c    |  55 +++++++++++
> >  security/integrity/ima/ima_queue.c    |  10 ++
> >  security/integrity/ima/ima_template.c | 171 ++++++++++++++++++++++++++++++++++
> >  6 files changed, 249 insertions(+)
> >  create mode 100644 security/integrity/ima/ima_kexec.c
> > 
> > diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> > index c34599f..c0ce7b1 100644
> > --- a/security/integrity/ima/Makefile
> > +++ b/security/integrity/ima/Makefile
> > @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o
> >  ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
> >  	 ima_policy.o ima_template.o ima_template_lib.o ima_buffer.o
> >  ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> > +ima-$(CONFIG_KEXEC_FILE) += ima_kexec.o
> >  obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index b5728da..84e8d36 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -102,6 +102,13 @@ struct ima_queue_entry {
> >  };
> >  extern struct list_head ima_measurements;	/* list of all measurements */
> >  
> > +/* Some details preceding the binary serialized measurement list */
> > +struct ima_kexec_hdr {
> > +	unsigned short version;
> > +	unsigned long buffer_size;
> > +	unsigned long count;
> > +} __packed;
> 
> Unless there is no real need for this structure to be packed i suggest dropping 
> the attribute.  When referenced through pointer 32bit ARM and MIPS (and likely 
> all other 32bit RISC CPUs) use rather inefficient byte loads and stores.
> 
> Worse, if, for example, ->count is going to be read/written concurrently from 
> multiple threads we get torn loads/stores thus losing atomicity of the access.

This header is used to prefix the serialized binary measurement list
with some meta-data about the measurement list being restored.
Unfortunately kexec_get_handover_buffer() returns the segment size, not
the actual ima measurement list buffer size.  The header info is set
using memcpy() once in ima_dump_measurement_list() and then the fields
are used in ima_restore_measurement_list() to verify the buffer.

The binary runtime measurement list is packed, so the other two
structures - binary_hdr_v1 and binary_data_v1 - must be packed.  Does it
make sense for this header not to be packed as well?  Would copying the
header fields to local variables before being used solve your concern?

Remember this code is used once on the kexec execute and again on
reboot.

Mimi