Message-Id: <1560267426.4464.173.camel@linux.ibm.com>
Date:   Tue, 11 Jun 2019 11:37:06 -0400
From:   Mimi Zohar <zohar@...ux.ibm.com>
To:     Prakhar Srivastava <prsriva02@...il.com>,
        linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     roberto.sassu@...wei.com, vgoyal@...hat.com
Subject: Re: [PATCH v7 0/3] add new ima hook ima_kexec_cmdline to measure
 kexec boot cmdline args

Hi Prakhar,

The patch/patch set title in the Subject line should not explain "how"
you add a new feature.  In this case an appropriate patch set title
would be, "Add support for measuring the boot command line".
 Similarly, the first patch in this patch set could be named "Define a
new IMA hook to measure the boot command line arguments".

On Thu, 2019-06-06 at 17:23 -0700, Prakhar Srivastava wrote:
> The motive behind the patch series is to measure the boot cmdline args
> used for soft reboot/kexec case.

When mentoring, I suggest starting out with a simple status statement
(eg. "The kexec boot command line arguments are not currently being
measured."), followed by the problem statement in the first paragraph.

> 
> For secure boot attestation, it is necessary to measure the kernel

Secure boot enforces local file data integrity.  The term here should
be "trusted boot attestation".

> command line and the kernel version. 

The original version of this patch set included the kernel version.  
This version is just measuring the boot command line arguments.

> For cold boot, the boot loader
> can be enhanced to measure these parameters.
> (https://mjg59.dreamwidth.org/48897.html)
> However, for attestation across soft reboot boundary, these values 
> also need to be measured during kexec_file_load.
> 
> Currently for Kexec(kexec_file_load)/soft reboot scenario the boot cmdline
> args for the next kernel are not measured. For 
> normal case of boot/hardreboot the cmdline args are measured into the TPM.
> The hash of boot command line is calculated and added to the current 
> running kernel's measurement list.
> On a soft reboot like kexec, no cmdline arguments measurement takes place.
> 
> To achive the above the patch series does the following
>   -adds a new ima hook: ima_kexec_cmdline which measures the cmdline args
>    into the ima log, behind a new ima policy entry KEXEC_CMDLINE.
>   -since the cmldine args cannot be appraised, a new template field(buf) is
>    added. The template field contains the buffer passed(cmldine args), which
>    can be used to appraise/attest at a later stage.
>   -call the ima_kexec_cmdline(...) hook from kexec_file_load call.
> 
> The ima logs need to be carried over to the next kernel, which will be followed
> up by other patchsets for x86_64 and arm64.
> 
> The kexec cmdline hash

^stored in the "d-ng" field of the template data

> can be verified using

> sudo cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | 
>   grep  kexec-cmdline | cut -d' ' -f 6 | xxd -r -p | sha256sum

Until per policy template field rule support is added, a template name
needs to be defined.  Please define "ima-buf" as:
{.name = "ima-buf", .fmt = "d-ng|n-ng|buf"}

I'm still seeing some scripts/checkpatch "WARNING: line over 80
characters".  scripts/Lindent should provide the correct way of
formatting these lines.

Some people feel that references to Lindent should be removed, but I
tend to agree with the Documentation/hwmon/submitting-patches.rst
comment pertaining to scripts/Lindent.

"* Running your patch or driver file(s) through checkpatch does not
mean its formatting is clean. If unsure about formatting in your new
driver, run it through Lindent. Lindent is not perfect, and you may
have to do some minor cleanup, but it is a good start."

Examples of where the line formatting is off is the call to
ima_get_action() in process_buffer_measurement() and the call to
process_buffer_measurement() in ima_kexec_cmdline().

thanks,

Mimi

> 
> Changelog:
> V7:
>   - rebased to next-queued-testing
>   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/log/?h=next-queued-testing
> 
> V6:
>   -add a new ima hook and policy to measure the cmdline
>     args(ima_kexec_cmdline)
>   -add a new template field buf to contain the buffer measured.
>   [suggested by Mimi Zohar]
>    add new fields to ima_event_data to store/read buffer data.
>   [suggested by Roberto]
>   -call ima_kexec_cmdline from kexec_file_load path
> 
> v5:
>   -add a new ima hook and policy to measure the cmdline
>     args(ima_kexec_cmdline)
>   -add a new template field buf to contain the buffer measured.
>     [suggested by Mimi Zohar]
>   -call ima_kexec_cmdline from kexec_file_load path
> 
> v4:
>   - per feedback from LSM community, removed the LSM hook and renamed the
>     IMA policy to KEXEC_CMDLINE
> 
> v3: (rebase changes to next-general)
>   - Add policy checks for buffer[suggested by Mimi Zohar]
>   - use the IMA_XATTR to add buffer
>   - Add kexec_cmdline used for kexec file load
>   - Add an LSM hook to allow usage by other LSM.[suggestd by Mimi Zohar]
> 
> v2:
>   - Add policy checks for buffer[suggested by Mimi Zohar]
>   - Add an LSM hook to allow usage by other LSM.[suggestd by Mimi Zohar]
>   - use the IMA_XATTR to add buffer instead of sig template
> 
> v1:
>   -Add kconfigs to control the ima_buffer_check
>   -measure the cmdline args suffixed with the kernel file name
>   -add the buffer to the template sig field.
> 
> Prakhar Srivastava (3):
>   Add a new ima hook ima_kexec_cmdline to measure cmdline args
>   add a new ima template field buf
>   call ima_kexec_cmdline to measure the cmdline args
> 
>  Documentation/ABI/testing/ima_policy      |  1 +
>  Documentation/security/IMA-templates.rst  |  2 +-
>  include/linux/ima.h                       |  2 +
>  kernel/kexec_file.c                       |  8 ++-
>  security/integrity/ima/ima.h              |  3 +
>  security/integrity/ima/ima_api.c          |  5 +-
>  security/integrity/ima/ima_init.c         |  2 +-
>  security/integrity/ima/ima_main.c         | 80 +++++++++++++++++++++++
>  security/integrity/ima/ima_policy.c       |  9 +++
>  security/integrity/ima/ima_template.c     |  2 +
>  security/integrity/ima/ima_template_lib.c | 20 ++++++
>  security/integrity/ima/ima_template_lib.h |  4 ++
>  12 files changed, 131 insertions(+), 7 deletions(-)
> 

Powered by blists - more mailing lists