[RESEND][PATCH] Documentation: added order requirement for ima_hash=

[RESEND][PATCH] Documentation: added order requirement for ima_hash=

[GUO Zihua]

From: Guo Zihua <guozihua@huawei.com>

Commandline parameter ima_hash= and ima_template= has order requirement
for them to work correctly together. Namely ima_hash= must be
specified after ima_template=, otherwise ima_template= will be ignored.

The reason is that when handling ima_hash=, ima template would be set to
the default value if it has not been initialized already, and that value
cannot be changed afterwards by ima_template=.

This patch adds this limitation to the documentation.

Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>
Signed-off-by: Guo Zihua <guozihua@huawei.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index f5a27f067db9..1b5aa6ca65f8 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1843,6 +1843,10 @@
 			The list of supported hash algorithms is defined
 			in crypto/hash_info.h.
 
+			This parameter must be specified after ima_template=,
+			as it would set the default template and that cannot be
+			changed by ima_template= afterwards.
+
 	ima_policy=	[IMA]
 			The builtin policies to load during IMA setup.
 			Format: "tcb | appraise_tcb | secure_boot |
@@ -1879,6 +1883,9 @@
 			Formats: { "ima" | "ima-ng" | "ima-sig" }
 			Default: "ima-ng"
 
+			This parameter must be specified before ima_hash=.
+			Please refer to ima_hash= for further explanation.
+
 	ima_template_fmt=
 			[IMA] Define a custom template format.
 			Format: { "field1|...|fieldN" }
-- 
2.17.1

Re: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Jonathan Corbet]

GUO Zihua <guozihua@huawei.com> writes:

> From: Guo Zihua <guozihua@huawei.com>
>
> Commandline parameter ima_hash= and ima_template= has order requirement
> for them to work correctly together. Namely ima_hash= must be
> specified after ima_template=, otherwise ima_template= will be ignored.
>
> The reason is that when handling ima_hash=, ima template would be set to
> the default value if it has not been initialized already, and that value
> cannot be changed afterwards by ima_template=.
>
> This patch adds this limitation to the documentation.
>
> Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Guo Zihua <guozihua@huawei.com>

I've applied this, but I'm wondering: where did this review take place?
I can't find it on the lists...

Thanks,

jon

Re: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Guozihua (Scott)]

On 2022/1/26 8:14, Jonathan Corbet wrote:
> GUO Zihua <guozihua@huawei.com> writes:
> 
>> From: Guo Zihua <guozihua@huawei.com>
>>
>> Commandline parameter ima_hash= and ima_template= has order requirement
>> for them to work correctly together. Namely ima_hash= must be
>> specified after ima_template=, otherwise ima_template= will be ignored.
>>
>> The reason is that when handling ima_hash=, ima template would be set to
>> the default value if it has not been initialized already, and that value
>> cannot be changed afterwards by ima_template=.
>>
>> This patch adds this limitation to the documentation.
>>
>> Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>
>> Signed-off-by: Guo Zihua <guozihua@huawei.com>
> 
> I've applied this, but I'm wondering: where did this review take place?
> I can't find it on the lists...
> 
> Thanks,
> 
> jon
> .

Hi Jonathan,

Thank you very much and sorry for the confusion here. The reviewed by is 
more like a face-to-face peer review and I would like to mention that in 
the patch. If this is problematic I would stop doing that from now on.

-- 
Best
GUO Zihua

Re: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Mimi Zohar]

On Tue, 2022-01-25 at 17:02 +0800, GUO Zihua wrote:
> From: Guo Zihua <guozihua@huawei.com>
> 
> Commandline parameter ima_hash= and ima_template= has order requirement
> for them to work correctly together. Namely ima_hash= must be
> specified after ima_template=, otherwise ima_template= will be ignored.
> 
> The reason is that when handling ima_hash=, ima template would be set to
> the default value if it has not been initialized already, and that value
> cannot be changed afterwards by ima_template=.
> 
> This patch adds this limitation to the documentation.
> 
> Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Guo Zihua <guozihua@huawei.com>

This issue should be limited to the original "ima" template format,
which only supports hash algorithms of 20 bytes or less.  The "ima_ng"
template has been the default since larger digests and templates were
upstreamed back in Linux 3.13[1]. Do you really still have kernels
built with the original "ima" template?

[1] Refer to commit 4286587dccd4 ("ima: add Kconfig default measurement
list template").

thanks,

Mimi

> ---
>  Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index f5a27f067db9..1b5aa6ca65f8 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -1843,6 +1843,10 @@
>  			The list of supported hash algorithms is defined
>  			in crypto/hash_info.h.
>  
> +			This parameter must be specified after ima_template=,
> +			as it would set the default template and that cannot be
> +			changed by ima_template= afterwards.
> +
>  	ima_policy=	[IMA]
>  			The builtin policies to load during IMA setup.
>  			Format: "tcb | appraise_tcb | secure_boot |
> @@ -1879,6 +1883,9 @@
>  			Formats: { "ima" | "ima-ng" | "ima-sig" }
>  			Default: "ima-ng"
>  
> +			This parameter must be specified before ima_hash=.
> +			Please refer to ima_hash= for further explanation.
> +
>  	ima_template_fmt=
>  			[IMA] Define a custom template format.
>  			Format: { "field1|...|fieldN" }

Re: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Guozihua (Scott)]

On 2022/1/26 9:07, Mimi Zohar wrote:
> On Tue, 2022-01-25 at 17:02 +0800, GUO Zihua wrote:
>> From: Guo Zihua <guozihua@huawei.com>
>>
>> Commandline parameter ima_hash= and ima_template= has order requirement
>> for them to work correctly together. Namely ima_hash= must be
>> specified after ima_template=, otherwise ima_template= will be ignored.
>>
>> The reason is that when handling ima_hash=, ima template would be set to
>> the default value if it has not been initialized already, and that value
>> cannot be changed afterwards by ima_template=.
>>
>> This patch adds this limitation to the documentation.
>>
>> Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>
>> Signed-off-by: Guo Zihua <guozihua@huawei.com>
> 
> This issue should be limited to the original "ima" template format,
> which only supports hash algorithms of 20 bytes or less.  The "ima_ng"
> template has been the default since larger digests and templates were
> upstreamed back in Linux 3.13[1]. Do you really still have kernels
> built with the original "ima" template?
> 
> [1] Refer to commit 4286587dccd4 ("ima: add Kconfig default measurement
> list template").
> 
> thanks,
> 
> Mimi

Hi Mimi,

The issue is that if ima_hash is specified before ima_template, 
ima_template will not work. Built-in default only affects which template 
will be loaded eventually.

For example, if the built-in default template is ima-ng and user would 
like to change it to ima-sig with sha512 by specifying "ima_hash=sha512 
ima_template=ima-sig" in command line, the result will be ima-ng with 
sha512, not ima-sig with sha512.

-- 
Best
GUO Zihua

> 
>> ---
>>   Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index f5a27f067db9..1b5aa6ca65f8 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1843,6 +1843,10 @@
>>   			The list of supported hash algorithms is defined
>>   			in crypto/hash_info.h.
>>   
>> +			This parameter must be specified after ima_template=,
>> +			as it would set the default template and that cannot be
>> +			changed by ima_template= afterwards.
>> +
>>   	ima_policy=	[IMA]
>>   			The builtin policies to load during IMA setup.
>>   			Format: "tcb | appraise_tcb | secure_boot |
>> @@ -1879,6 +1883,9 @@
>>   			Formats: { "ima" | "ima-ng" | "ima-sig" }
>>   			Default: "ima-ng"
>>   
>> +			This parameter must be specified before ima_hash=.
>> +			Please refer to ima_hash= for further explanation.
>> +
>>   	ima_template_fmt=
>>   			[IMA] Define a custom template format.
>>   			Format: { "field1|...|fieldN" }
> 
> 
> .

Re: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Mimi Zohar]

On Wed, 2022-01-26 at 10:28 +0800, Guozihua (Scott) wrote:
> 
> On 2022/1/26 9:07, Mimi Zohar wrote:
> > On Tue, 2022-01-25 at 17:02 +0800, GUO Zihua wrote:
> >> From: Guo Zihua <guozihua@huawei.com>
> >>
> >> Commandline parameter ima_hash= and ima_template= has order requirement
> >> for them to work correctly together. Namely ima_hash= must be
> >> specified after ima_template=, otherwise ima_template= will be ignored.
> >>
> >> The reason is that when handling ima_hash=, ima template would be set to
> >> the default value if it has not been initialized already, and that value
> >> cannot be changed afterwards by ima_template=.
> >>
> >> This patch adds this limitation to the documentation.
> >>
> >> Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>
> >> Signed-off-by: Guo Zihua <guozihua@huawei.com>
> > 
> > This issue should be limited to the original "ima" template format,
> > which only supports hash algorithms of 20 bytes or less.  The "ima_ng"
> > template has been the default since larger digests and templates were
> > upstreamed back in Linux 3.13[1]. Do you really still have kernels
> > built with the original "ima" template?
> > 
> > [1] Refer to commit 4286587dccd4 ("ima: add Kconfig default measurement
> > list template").
> 
> Hi Mimi,
> 
> The issue is that if ima_hash is specified before ima_template, 
> ima_template will not work. Built-in default only affects which template 
> will be loaded eventually.
> 
> For example, if the built-in default template is ima-ng and user would 
> like to change it to ima-sig with sha512 by specifying "ima_hash=sha512 
> ima_template=ima-sig" in command line, the result will be ima-ng with 
> sha512, not ima-sig with sha512.

Ok.  Once the template name is set, ima_template_setup() doesn't allow
it to be reset.  This was probably done to set the template name to the
first occurance of "ima_template=" on the boot command line.  This
concern could be addressed by defining a static local variable in
ima_template_setup().

So either documenting the ordering requirement, as you've done, or
allowing the template_name to be reset are fine.

thanks,

Mimi

Re: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Guozihua (Scott)]

On 2022/1/26 12:37, Mimi Zohar wrote:
> On Wed, 2022-01-26 at 10:28 +0800, Guozihua (Scott) wrote:
>>
>> On 2022/1/26 9:07, Mimi Zohar wrote:
>>> On Tue, 2022-01-25 at 17:02 +0800, GUO Zihua wrote:
>>>> From: Guo Zihua <guozihua@huawei.com>
>>>>
>>>> Commandline parameter ima_hash= and ima_template= has order requirement
>>>> for them to work correctly together. Namely ima_hash= must be
>>>> specified after ima_template=, otherwise ima_template= will be ignored.
>>>>
>>>> The reason is that when handling ima_hash=, ima template would be set to
>>>> the default value if it has not been initialized already, and that value
>>>> cannot be changed afterwards by ima_template=.
>>>>
>>>> This patch adds this limitation to the documentation.
>>>>
>>>> Reviewed-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>> Signed-off-by: Guo Zihua <guozihua@huawei.com>
>>>
>>> This issue should be limited to the original "ima" template format,
>>> which only supports hash algorithms of 20 bytes or less.  The "ima_ng"
>>> template has been the default since larger digests and templates were
>>> upstreamed back in Linux 3.13[1]. Do you really still have kernels
>>> built with the original "ima" template?
>>>
>>> [1] Refer to commit 4286587dccd4 ("ima: add Kconfig default measurement
>>> list template").
>>
>> Hi Mimi,
>>
>> The issue is that if ima_hash is specified before ima_template,
>> ima_template will not work. Built-in default only affects which template
>> will be loaded eventually.
>>
>> For example, if the built-in default template is ima-ng and user would
>> like to change it to ima-sig with sha512 by specifying "ima_hash=sha512
>> ima_template=ima-sig" in command line, the result will be ima-ng with
>> sha512, not ima-sig with sha512.
> 
> Ok.  Once the template name is set, ima_template_setup() doesn't allow
> it to be reset.  This was probably done to set the template name to the
> first occurance of "ima_template=" on the boot command line.  This
> concern could be addressed by defining a static local variable in
> ima_template_setup().
> 
> So either documenting the ordering requirement, as you've done, or
> allowing the template_name to be reset are fine.
> 
> thanks,
> 
> Mimi
> 
> .

The main issue lies in ima_template_desc_current called by hash_setup, 
which does not just read ima_template global variable, but also tries to 
set it if that hasn't been done already. Causing ima_template_setup to quit.

-- 
Best
GUO Zihua

Re: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Mimi Zohar]

On Wed, 2022-01-26 at 15:41 +0800, Guozihua (Scott) wrote:
> 
> 
> The main issue lies in ima_template_desc_current called by hash_setup, 
> which does not just read ima_template global variable, but also tries to 
> set it if that hasn't been done already. Causing ima_template_setup to quit.

Right, which calls ima_init_template_list().  So part of the solution
could be to conditionally call ima_init_template_list()
in ima_template_setup().

-       if (ima_template)
-               return 1;
-
-       ima_init_template_list();
+       if (!ima_template
+               ima_init_template_list();

Roberto, what do you think?

thanks,

Mimi

RE: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Wednesday, January 26, 2022 1:48 PM
> On Wed, 2022-01-26 at 15:41 +0800, Guozihua (Scott) wrote:
> >
> >
> > The main issue lies in ima_template_desc_current called by hash_setup,
> > which does not just read ima_template global variable, but also tries to
> > set it if that hasn't been done already. Causing ima_template_setup to quit.
> 
> Right, which calls ima_init_template_list().  So part of the solution
> could be to conditionally call ima_init_template_list()
> in ima_template_setup().
> 
> -       if (ima_template)
> -               return 1;
> -
> -       ima_init_template_list();
> +       if (!ima_template
> +               ima_init_template_list();
> 
> Roberto, what do you think?

Hi Mimi

I think we wanted to prevent to set a digest algorithm
incompatible with the chosen template.

If we have in the kernel command line:

ima_template=ima ima_hash=sha256

ima_hash_algo would be set to HASH_ALGO_SHA1 despite
the user choice and the template would be set to 'ima'.

In the opposite case:	

ima_hash=sha256 ima_template=ima

if the default template is 'ima', then ima_hash_algo would be
set to HASH_ALGO_SHA1. Otherwise, it would be
HASH_ALGO_SHA256. If we allow the template to be set after
the digest algorithm is evaluated, the template selection will
be rejected if the algorithm is incompatible with the template.

I'm trying to remember why we still have the digest recalculation
in ima_eventdigest_init(). Maybe the only possibility is if we
set the template from the policy?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

> thanks,
> 
> Mimi

Re: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Mimi Zohar]

On Wed, 2022-01-26 at 13:24 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Wednesday, January 26, 2022 1:48 PM
> > On Wed, 2022-01-26 at 15:41 +0800, Guozihua (Scott) wrote:
> > >
> > >
> > > The main issue lies in ima_template_desc_current called by hash_setup,
> > > which does not just read ima_template global variable, but also tries to
> > > set it if that hasn't been done already. Causing ima_template_setup to quit.
> > 
> > Right, which calls ima_init_template_list().  So part of the solution
> > could be to conditionally call ima_init_template_list()
> > in ima_template_setup().
> > 
> > -       if (ima_template)
> > -               return 1;
> > -
> > -       ima_init_template_list();
> > +       if (!ima_template
> > +               ima_init_template_list();
> > 
> > Roberto, what do you think?
> 
> Hi Mimi
> 
> I think we wanted to prevent to set a digest algorithm
> incompatible with the chosen template.
> 
> If we have in the kernel command line:
> 
> ima_template=ima ima_hash=sha256
> 
> ima_hash_algo would be set to HASH_ALGO_SHA1 despite
> the user choice and the template would be set to 'ima'.
> 
> In the opposite case:	
> 
> ima_hash=sha256 ima_template=ima
> 
> if the default template is 'ima', then ima_hash_algo would be
> set to HASH_ALGO_SHA1. Otherwise, it would be
> HASH_ALGO_SHA256. If we allow the template to be set after
> the digest algorithm is evaluated, the template selection will
> be rejected if the algorithm is incompatible with the template.

The only time that would occur is in the unlikely case that the
template is being set to "ima".   That sounds reasonable.  In fact we
should consider preventing the template format being set to "ima".

> 
> I'm trying to remember why we still have the digest recalculation
> in ima_eventdigest_init(). Maybe the only possibility is if we
> set the template from the policy?

The recalculation was relatively recently added in commit 6cc7c266e5b4
("ima: Call ima_calc_boot_aggregate() in ima_eventdigest_init()").

thanks,

Mimi

RE: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Wednesday, January 26, 2022 3:35 PM
> On Wed, 2022-01-26 at 13:24 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Wednesday, January 26, 2022 1:48 PM
> > > On Wed, 2022-01-26 at 15:41 +0800, Guozihua (Scott) wrote:
> > > >
> > > >
> > > > The main issue lies in ima_template_desc_current called by hash_setup,
> > > > which does not just read ima_template global variable, but also tries to
> > > > set it if that hasn't been done already. Causing ima_template_setup to quit.
> > >
> > > Right, which calls ima_init_template_list().  So part of the solution
> > > could be to conditionally call ima_init_template_list()
> > > in ima_template_setup().
> > >
> > > -       if (ima_template)
> > > -               return 1;
> > > -
> > > -       ima_init_template_list();
> > > +       if (!ima_template
> > > +               ima_init_template_list();
> > >
> > > Roberto, what do you think?
> >
> > Hi Mimi
> >
> > I think we wanted to prevent to set a digest algorithm
> > incompatible with the chosen template.
> >
> > If we have in the kernel command line:
> >
> > ima_template=ima ima_hash=sha256
> >
> > ima_hash_algo would be set to HASH_ALGO_SHA1 despite
> > the user choice and the template would be set to 'ima'.
> >
> > In the opposite case:
> >
> > ima_hash=sha256 ima_template=ima
> >
> > if the default template is 'ima', then ima_hash_algo would be
> > set to HASH_ALGO_SHA1. Otherwise, it would be
> > HASH_ALGO_SHA256. If we allow the template to be set after
> > the digest algorithm is evaluated, the template selection will
> > be rejected if the algorithm is incompatible with the template.
> 
> The only time that would occur is in the unlikely case that the
> template is being set to "ima".   That sounds reasonable.  In fact we
> should consider preventing the template format being set to "ima".

Ok.

> > I'm trying to remember why we still have the digest recalculation
> > in ima_eventdigest_init(). Maybe the only possibility is if we
> > set the template from the policy?
> 
> The recalculation was relatively recently added in commit 6cc7c266e5b4
> ("ima: Call ima_calc_boot_aggregate() in ima_eventdigest_init()").

There is also recalculation for the file digest:

        hash.hdr.algo = ima_template_hash_algo_allowed(ima_hash_algo) ?
            ima_hash_algo : HASH_ALGO_SHA1;
        result = ima_calc_file_hash(event_data->file, &hash.hdr);

I understood that Jonathan already applied the patch. If it is possible
to make a new patch according to your suggestion, I would ask Zihua
to do that.

Jonathan, would it be fine for you to discard this patch?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

> thanks,
> 
> Mimi

RE: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Jonathan Corbet]

Roberto Sassu <roberto.sassu@huawei.com> writes:

> I understood that Jonathan already applied the patch. If it is possible
> to make a new patch according to your suggestion, I would ask Zihua
> to do that.
>
> Jonathan, would it be fine for you to discard this patch?

OK, I will drop it.

jon

Re: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Guozihua (Scott)]

On 2022/1/26 22:43, Roberto Sassu wrote:
>> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
>> Sent: Wednesday, January 26, 2022 3:35 PM
>> On Wed, 2022-01-26 at 13:24 +0000, Roberto Sassu wrote:
>>>> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
>>>> Sent: Wednesday, January 26, 2022 1:48 PM
>>>> On Wed, 2022-01-26 at 15:41 +0800, Guozihua (Scott) wrote:
>>>>>
>>>>>
>>>>> The main issue lies in ima_template_desc_current called by hash_setup,
>>>>> which does not just read ima_template global variable, but also tries to
>>>>> set it if that hasn't been done already. Causing ima_template_setup to quit.
>>>>
>>>> Right, which calls ima_init_template_list().  So part of the solution
>>>> could be to conditionally call ima_init_template_list()
>>>> in ima_template_setup().
>>>>
>>>> -       if (ima_template)
>>>> -               return 1;
>>>> -
>>>> -       ima_init_template_list();
>>>> +       if (!ima_template
>>>> +               ima_init_template_list();
>>>>
>>>> Roberto, what do you think?
>>>
>>> Hi Mimi
>>>
>>> I think we wanted to prevent to set a digest algorithm
>>> incompatible with the chosen template.
>>>
>>> If we have in the kernel command line:
>>>
>>> ima_template=ima ima_hash=sha256
>>>
>>> ima_hash_algo would be set to HASH_ALGO_SHA1 despite
>>> the user choice and the template would be set to 'ima'.
>>>
>>> In the opposite case:
>>>
>>> ima_hash=sha256 ima_template=ima
>>>
>>> if the default template is 'ima', then ima_hash_algo would be
>>> set to HASH_ALGO_SHA1. Otherwise, it would be
>>> HASH_ALGO_SHA256. If we allow the template to be set after
>>> the digest algorithm is evaluated, the template selection will
>>> be rejected if the algorithm is incompatible with the template.
>>
>> The only time that would occur is in the unlikely case that the
>> template is being set to "ima".   That sounds reasonable.  In fact we
>> should consider preventing the template format being set to "ima".
> 
> Ok.
> 
>>> I'm trying to remember why we still have the digest recalculation
>>> in ima_eventdigest_init(). Maybe the only possibility is if we
>>> set the template from the policy?
>>
>> The recalculation was relatively recently added in commit 6cc7c266e5b4
>> ("ima: Call ima_calc_boot_aggregate() in ima_eventdigest_init()").
> 
> There is also recalculation for the file digest:
> 
>          hash.hdr.algo = ima_template_hash_algo_allowed(ima_hash_algo) ?
>              ima_hash_algo : HASH_ALGO_SHA1;
>          result = ima_calc_file_hash(event_data->file, &hash.hdr);
> 
> I understood that Jonathan already applied the patch. If it is possible
> to make a new patch according to your suggestion, I would ask Zihua
> to do that.
Hi Mimi and Roberto,

I understand that the solution proposed here is to decommission template 
"ima" and potentially removing related algo checks altogether?

-- 
Best
GUO Zihua

Re: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Mimi Zohar]

On Thu, 2022-01-27 at 14:35 +0800, Guozihua (Scott) wrote:
> 
> On 2022/1/26 22:43, Roberto Sassu wrote:
> >> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> >> Sent: Wednesday, January 26, 2022 3:35 PM
> >> On Wed, 2022-01-26 at 13:24 +0000, Roberto Sassu wrote:
> >>>> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> >>>> Sent: Wednesday, January 26, 2022 1:48 PM
> >>>> On Wed, 2022-01-26 at 15:41 +0800, Guozihua (Scott) wrote:
> >>>>>
> >>>>>
> >>>>> The main issue lies in ima_template_desc_current called by hash_setup,
> >>>>> which does not just read ima_template global variable, but also tries to
> >>>>> set it if that hasn't been done already. Causing ima_template_setup to quit.
> >>>>
> >>>> Right, which calls ima_init_template_list().  So part of the solution
> >>>> could be to conditionally call ima_init_template_list()
> >>>> in ima_template_setup().
> >>>>
> >>>> -       if (ima_template)
> >>>> -               return 1;
> >>>> -
> >>>> -       ima_init_template_list();
> >>>> +       if (!ima_template
> >>>> +               ima_init_template_list();
> >>>>
> >>>> Roberto, what do you think?
> >>>
> >>> Hi Mimi
> >>>
> >>> I think we wanted to prevent to set a digest algorithm
> >>> incompatible with the chosen template.
> >>>
> >>> If we have in the kernel command line:
> >>>
> >>> ima_template=ima ima_hash=sha256
> >>>
> >>> ima_hash_algo would be set to HASH_ALGO_SHA1 despite
> >>> the user choice and the template would be set to 'ima'.
> >>>
> >>> In the opposite case:
> >>>
> >>> ima_hash=sha256 ima_template=ima
> >>>
> >>> if the default template is 'ima', then ima_hash_algo would be
> >>> set to HASH_ALGO_SHA1. Otherwise, it would be
> >>> HASH_ALGO_SHA256. If we allow the template to be set after
> >>> the digest algorithm is evaluated, the template selection will
> >>> be rejected if the algorithm is incompatible with the template.
> >>
> >> The only time that would occur is in the unlikely case that the
> >> template is being set to "ima".   That sounds reasonable.  In fact we
> >> should consider preventing the template format being set to "ima".
> > 
> > Ok.

< snip >

> 
> I understand that the solution proposed here is to decommission template 
> "ima" and potentially removing related algo checks altogether?

Eventually we might decide to do that, but right now we just want to
address not being able to set "ima_template" after setting "ima_hash".

thanks,

Mimi

Re: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Guozihua (Scott)]

On 2022/1/27 20:18, Mimi Zohar wrote:
> On Thu, 2022-01-27 at 14:35 +0800, Guozihua (Scott) wrote:
> 
>>
>> I understand that the solution proposed here is to decommission template
>> "ima" and potentially removing related algo checks altogether?
> 
> Eventually we might decide to do that, but right now we just want to
> address not being able to set "ima_template" after setting "ima_hash".
> 
> thanks,
> 
> Mimi
> 
> 
> .

Sure! I would come up with a solution.

-- 
Best
GUO Zihua

RE: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Wednesday, January 26, 2022 1:48 PM
> On Wed, 2022-01-26 at 15:41 +0800, Guozihua (Scott) wrote:
> >
> >
> > The main issue lies in ima_template_desc_current called by hash_setup,
> > which does not just read ima_template global variable, but also tries to
> > set it if that hasn't been done already. Causing ima_template_setup to quit.
> 
> Right, which calls ima_init_template_list().  So part of the solution
> could be to conditionally call ima_init_template_list()
> in ima_template_setup().
> 
> -       if (ima_template)
> -               return 1;
> -
> -       ima_init_template_list();
> +       if (!ima_template
> +               ima_init_template_list();

Hi Mimi

is it still necessary to call ima_init_template_list() in
template_setup()? I saw it is called in init_ima().

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

> Roberto, what do you think?
> 
> thanks,
> 
> Mimi

Re: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Mimi Zohar]

On Fri, 2022-01-28 at 10:24 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Wednesday, January 26, 2022 1:48 PM
> > On Wed, 2022-01-26 at 15:41 +0800, Guozihua (Scott) wrote:
> > >
> > >
> > > The main issue lies in ima_template_desc_current called by hash_setup,
> > > which does not just read ima_template global variable, but also tries to
> > > set it if that hasn't been done already. Causing ima_template_setup to quit.
> > 
> > Right, which calls ima_init_template_list().  So part of the solution
> > could be to conditionally call ima_init_template_list()
> > in ima_template_setup().
> > 
> > -       if (ima_template)
> > -               return 1;
> > -
> > -       ima_init_template_list();
> > +       if (!ima_template
> > +               ima_init_template_list();
> 
> 
> is it still necessary to call ima_init_template_list() in
> template_setup()? I saw it is called in init_ima().

All of these options are at __setup().

thanks,

Mimi

RE: [RESEND][PATCH] Documentation: added order requirement for ima_hash=

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, January 28, 2022 3:34 PM
> On Fri, 2022-01-28 at 10:24 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Wednesday, January 26, 2022 1:48 PM
> > > On Wed, 2022-01-26 at 15:41 +0800, Guozihua (Scott) wrote:
> > > >
> > > >
> > > > The main issue lies in ima_template_desc_current called by hash_setup,
> > > > which does not just read ima_template global variable, but also tries to
> > > > set it if that hasn't been done already. Causing ima_template_setup to quit.
> > >
> > > Right, which calls ima_init_template_list().  So part of the solution
> > > could be to conditionally call ima_init_template_list()
> > > in ima_template_setup().
> > >
> > > -       if (ima_template)
> > > -               return 1;
> > > -
> > > -       ima_init_template_list();
> > > +       if (!ima_template
> > > +               ima_init_template_list();
> >
> >
> > is it still necessary to call ima_init_template_list() in
> > template_setup()? I saw it is called in init_ima().
> 
> All of these options are at __setup().

Yes. ima_init_template_list() should be called before
lookup_template_desc().

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua

> thanks,
> 
> Mimi