Re: [PATCH v2 3/3] kexec: extend kexec_file_load system call

[Thiago Jung Bauermann]

Hi,

Am Dienstag, 26 Juli 2016, 21:24:29 schrieb Thiago Jung Bauermann:
> Notes:
> This is a new version of the last patch in this series which adds
> a function where each architecture can verify if the DTB is safe
> to load:
> 
> int __weak arch_kexec_verify_buffer(enum kexec_file_type type,
> const void *buf,
> unsigned long size)
> {
> return -EINVAL;
> }
> 
> I will then provide an implementation in my powerpc patch series
> which checks that the DTB only contains nodes and properties from a
> whitelist. arch_kexec_kernel_image_load will copy these properties
> to the device tree blob the kernel was booted with (and perform
> other changes such as setting /chosen/bootargs, of course).

Is this approach ok? If so, I'll post a patch next week adding an 
arch_kexec_verify_buffer hook for powerpc to enforce the whitelist, and also 
a new version of the patches implementing kexec_file_load for powerpc on top 
of this series.

Eric, does this address your concerns?

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Petko Manolov]

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 
> > > ---
> > >  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

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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 
> > ---
> >  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


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

[Petko Manolov]

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 
> ---
>  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.


Petko

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec