Re: [PATCH v5 2/7] IMA: update process_buffer_measurement to measure buffer hash

[Tushar Sugandhi]

On 2020-11-12 2:19 p.m., Mimi Zohar wrote:
> On Thu, 2020-11-12 at 13:47 -0800, Tushar Sugandhi wrote:
> > > On Sun, 2020-11-01 at 14:26 -0800, Tushar Sugandhi wrote:
> > > > process_buffer_measurement() currently only measures the input buffer.
> > > > In case of SeLinux policy measurement, the policy being measured could
> > > > be large (several MB). This may result in a large entry in IMA
> > > > measurement log.
> > >
> > > SELinux is an example of measuring large buffer data.  Please rewrite
> > > this patch description (and the other patch descriptions in this patch
> > > set) without using the example to describe its purpose [1].
> > >
> > > In this case, you might say,
> > >
> > > The original IMA buffer data measurement sizes were small (e.g. boot
> > > command line), but new buffer data measurement use cases are a lot
> > > larger.  Just as IMA measures the file data hash, not the file data,
> > > IMA should similarly support measuring the buffer data hash.
> > Sure. Thanks a lot for giving an example wording for us. Will update.
> > > > Introduce a boolean parameter measure_buf_hash to support measuring
> > > > hash of a buffer, which would be much smaller, instead of the buffer
> > > > itself.
> > >
> > > > To use the functionality introduced in this patch, the attestation
> > > > client and the server changes need to go hand in hand. The
> > > > client/kernel would know what data is being measured as-is
> > > > (e.g. KEXEC_CMDLINE), and what data has it’s hash measured (e.g. SeLinux
> > > > Policy). And the attestation server should verify data/hash accordingly.
> > > >
> > > > Just like the data being measured in other cases, the attestation server
> > > > will know what are possible values of the large buffers being measured.
> > > > e.g. the possible valid SeLinux policy values that are being pushed to
> > > > the client. The attestation server will have to maintain the hash of
> > > > those buffer values.
> > >
> > > Each patch in the patch set builds upon the previous one.   (Think of
> > > it as a story, where each chapter builds upon the previous ones.)
> > > With rare exceptions, should patches reference subsequent patches. [2]
> > >
> > > [1] Refer to Documentation/process/submitting-patches.rst
> > > [2] Refer to the section "8) Commenting" in
> > > Documentation/process/coding-style.rst
> > I am not sure if you have any concerns about the last two paragraphs.
> > The description about the attestation client and server (the last two
> > paragraphs) was added for information/clarification purpose only, as per
> > your feedback on previous iterations. The subsequent patches don’t have
> > any code pertaining to attestation client/server.
> >
> > *Question*
> > Maybe the last two paragraphs are confusing/redundant. Could you please
> > let me know if I should remove the above two paragraphs altogether?
> > (starting with “To use the functionality introduced in this patch ...”)
> >
> > If we decide to keep the paragraphs, I will remove the specific examples
> > (KEXEC_CMDLINE, SeLinux etc.) as you mentioned elsewhere.
>
> Instead of the above two paragraphs, perhaps explain how measuring the
> file data hash differs from measuring the buffer data hash.  Keep the
> explanation generic, short and simple.
>
> Mimi
Will do. Thanks for the quick response Mimi.
I also have some clarification questions on the other patches in this
series as well.

Would really appreciate if you could help us get clarification on those.

Thanks a lot again.

~Tushar