On 16-08-05 09:34:38, Mimi Zohar wrote:
> 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 at linux.vnet.ibm.com>
> > > ---
> > > 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.
As long as there is no concurrent reads/writes this should be OK.
> 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?
Copying to aligned variables would be necessary only if:
a) some sort of atomicity is needed, and/or
?) speed is of concern;
> Remember this code is used once on the kexec execute and again on reboot.
If we don't need a) _and_ b) then you don't need to bother.
Petko
