| Message ID | 20240122183804.3293904-7-tusharsu@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: kexec: measure events between kexec load and execute | expand |
On 1/22/24 13:38, Tushar Sugandhi wrote: > The extra memory allocated for carrying the IMA measurement list across > kexec is hardcoded as half a PAGE. Make it configurable. > > Define a Kconfig option, IMA_KEXEC_EXTRA_MEMORY_KB, to configure the > extra memory (in kb) to be allocated for IMA measurements added during > kexec soft reboot. Ensure the default value of the option is set such > that extra half a page of memory for additional measurements is allocated > to maintain backwards compatibility. > > Update ima_add_kexec_buffer() function to allocate memory based on the > Kconfig option value, rather than the currently hardcoded one. > > Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> > --- > security/integrity/ima/Kconfig | 11 +++++++++++ > security/integrity/ima/ima_kexec.c | 15 ++++++++++----- > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > index 60a511c6b583..fc103288852b 100644 > --- a/security/integrity/ima/Kconfig > +++ b/security/integrity/ima/Kconfig > @@ -338,3 +338,14 @@ config IMA_DISABLE_HTABLE > default n > help > This option disables htable to allow measurement of duplicate records. > + > +config IMA_KEXEC_EXTRA_MEMORY_KB > + int > + depends on IMA && IMA_KEXEC > + default 0 > + help > + IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be > + allocated (in kb) for IMA measurements added during kexec soft reboot. > + If set to the default value, an extra half page of memory for > + additional measurements will be allocated to maintain backwards > + compatibility. Is there really an issue with 'backwards compatibility' that the user needs to know about ? From looking at the code it seems more important that a bit of additional memory is reserved now to fit the kexec 'load' and 'exec' critical data events but that's not 'backwards compatibility'. Also mention this as a guidance on either how kexec load+exec need to be used or as a hint that it may potentially require a lot of memory? : The amount of extra memory that is necessary to carry all measurements across a kexec depends on memory requirements of the measurements taken between the kexec 'load' and 'exec' stages. The more measurements are taken, the more extra memory is required. Large amounts of extra memory can be avoided by running kexec 'load' and 'exec' in short sequence. > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c > index f26b5b342d84..c126aa6494e9 100644 > --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -121,6 +121,7 @@ void ima_add_kexec_buffer(struct kimage *image) > .buf_min = 0, .buf_max = ULONG_MAX, > .top_down = true }; > unsigned long binary_runtime_size; > + unsigned long extra_size; > > /* use more understandable variable names than defined in kbuf */ > void *kexec_buffer = NULL; > @@ -128,15 +129,19 @@ void ima_add_kexec_buffer(struct kimage *image) > int ret; > > /* > - * Reserve an extra half page of memory for additional measurements > - * added during the kexec load. > + * Reserve extra memory for measurements added during kexec. > */ > - binary_runtime_size = ima_get_binary_runtime_size(); > + if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0) > + extra_size = PAGE_SIZE / 2; > + else > + extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024; > + binary_runtime_size = ima_get_binary_runtime_size() + extra_size; > + > if (binary_runtime_size >= ULONG_MAX - PAGE_SIZE) > kexec_segment_size = ULONG_MAX; > else > - kexec_segment_size = ALIGN(ima_get_binary_runtime_size() + > - PAGE_SIZE / 2, PAGE_SIZE); > + kexec_segment_size = ALIGN(binary_runtime_size, PAGE_SIZE); > + > if ((kexec_segment_size == ULONG_MAX) || > ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) { > pr_err("Binary measurement list too large.\n"); Code LGTM
Thanks again Stefan for taking a look. On 1/23/24 11:02, Stefan Berger wrote: > > > On 1/22/24 13:38, Tushar Sugandhi wrote: >> The extra memory allocated for carrying the IMA measurement list across >> kexec is hardcoded as half a PAGE. Make it configurable. >> >> Define a Kconfig option, IMA_KEXEC_EXTRA_MEMORY_KB, to configure the >> extra memory (in kb) to be allocated for IMA measurements added during >> kexec soft reboot. Ensure the default value of the option is set such >> that extra half a page of memory for additional measurements is allocated >> to maintain backwards compatibility. >> >> Update ima_add_kexec_buffer() function to allocate memory based on the >> Kconfig option value, rather than the currently hardcoded one. >> >> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >> --- >> security/integrity/ima/Kconfig | 11 +++++++++++ >> security/integrity/ima/ima_kexec.c | 15 ++++++++++----- >> 2 files changed, 21 insertions(+), 5 deletions(-) >> >> diff --git a/security/integrity/ima/Kconfig >> b/security/integrity/ima/Kconfig >> index 60a511c6b583..fc103288852b 100644 >> --- a/security/integrity/ima/Kconfig >> +++ b/security/integrity/ima/Kconfig >> @@ -338,3 +338,14 @@ config IMA_DISABLE_HTABLE >> default n >> help >> This option disables htable to allow measurement of duplicate >> records. >> + >> +config IMA_KEXEC_EXTRA_MEMORY_KB >> + int >> + depends on IMA && IMA_KEXEC >> + default 0 >> + help >> + IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be >> + allocated (in kb) for IMA measurements added during kexec soft >> reboot. >> + If set to the default value, an extra half page of memory for >> + additional measurements will be allocated to maintain backwards >> + compatibility. > > Is there really an issue with 'backwards compatibility' that the user > needs to know about ? From looking at the code it seems more important > that a bit of additional memory is reserved now to fit the kexec 'load' > and 'exec' critical data events but that's not 'backwards compatibility'. > I was contemplating how to put the right description in place considering the conversation we had in v3 of this series[1]. Without that context[1] default 0 could be equally confusing to the end user. With the phrase 'backwards compatibility', I was trying to address the potential confusion around the default value 0 in the config - that it represents half-a-page as default. And that particular value choice ( half-a-page) is for backwards compatibility. You are right, I the user shouldn't care about it. But I had to start somewhere so that we can have this discussion on this thread. :) Let me know how this description looks after removing the phrase 'backwards compatibility': " IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be allocated (in kb) for IMA measurements added during kexec soft reboot. If set to the default value, an extra half a page of memory for those additional measurements will be allocated." Lastly, do you want me to add suggested-by and/or reviewed-by tag to this patch? Let me know. I will be happy to do so. Thanks, Tushar --- [1] Subject: Re: [PATCH v3 6/7] ima: configure memory to log events between kexec load and execute https://lore.kernel.org/all/a09613f2-0d9f-41a3-b78d-b1b1fd35614c@linux.microsoft.com/ On 1/12/24 09:44, Mimi Zohar wrote: > On Thu, 2024-01-11 at 12:52 -0800, Tushar Sugandhi wrote: > [...] >> If we go with the KBs approach - >> >> half-a-page translates to different KBs on different architectures. >> And setting the right default value in KBs which would translate to >> the desired half-a-page, on a given arch, inside the Kconfig seems >> fragile (as I mentioned in the context of Option A in my previous >> response. > > How about setting the default value to 0, indicating not to change the current > half page default. Any other value would be KBs, as Stefan suggested. > Thanks. That's way more elegant than the other alternatives. It's definitely doable in KConfig and handle the default in code appropriately. It may cause some confusion in the documentation of the config param. But it would be a wording issue, and we can work on it. > Also mention this as a guidance on either how kexec load+exec need to be > used or as a hint that it may potentially require a lot of memory? : > > The amount of extra memory that is necessary to carry all measurements > across a kexec depends on memory requirements of the measurements taken > between the kexec 'load' and 'exec' stages. The more measurements are > taken, the more extra memory is required. Large amounts of extra memory > can be avoided by running kexec 'load' and 'exec' in short sequence. > >> diff --git a/security/integrity/ima/ima_kexec.c >> b/security/integrity/ima/ima_kexec.c >> index f26b5b342d84..c126aa6494e9 100644 >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -121,6 +121,7 @@ void ima_add_kexec_buffer(struct kimage *image) >> .buf_min = 0, .buf_max = ULONG_MAX, >> .top_down = true }; >> unsigned long binary_runtime_size; >> + unsigned long extra_size; >> /* use more understandable variable names than defined in kbuf */ >> void *kexec_buffer = NULL; >> @@ -128,15 +129,19 @@ void ima_add_kexec_buffer(struct kimage *image) >> int ret; >> /* >> - * Reserve an extra half page of memory for additional measurements >> - * added during the kexec load. >> + * Reserve extra memory for measurements added during kexec. >> */ >> - binary_runtime_size = ima_get_binary_runtime_size(); >> + if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0) >> + extra_size = PAGE_SIZE / 2; >> + else >> + extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024; >> + binary_runtime_size = ima_get_binary_runtime_size() + extra_size; >> + >> if (binary_runtime_size >= ULONG_MAX - PAGE_SIZE) >> kexec_segment_size = ULONG_MAX; >> else >> - kexec_segment_size = ALIGN(ima_get_binary_runtime_size() + >> - PAGE_SIZE / 2, PAGE_SIZE); >> + kexec_segment_size = ALIGN(binary_runtime_size, PAGE_SIZE); >> + >> if ((kexec_segment_size == ULONG_MAX) || >> ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) { >> pr_err("Binary measurement list too large.\n"); > > Code LGTM
On 1/23/24 16:19, Tushar Sugandhi wrote: > Thanks again Stefan for taking a look. > > On 1/23/24 11:02, Stefan Berger wrote: >> >> >> On 1/22/24 13:38, Tushar Sugandhi wrote: >>> The extra memory allocated for carrying the IMA measurement list across >>> kexec is hardcoded as half a PAGE. Make it configurable. >>> >>> Define a Kconfig option, IMA_KEXEC_EXTRA_MEMORY_KB, to configure the >>> extra memory (in kb) to be allocated for IMA measurements added during >>> kexec soft reboot. Ensure the default value of the option is set such >>> that extra half a page of memory for additional measurements is >>> allocated >>> to maintain backwards compatibility. >>> >>> Update ima_add_kexec_buffer() function to allocate memory based on the >>> Kconfig option value, rather than the currently hardcoded one. >>> >>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> >>> --- >>> security/integrity/ima/Kconfig | 11 +++++++++++ >>> security/integrity/ima/ima_kexec.c | 15 ++++++++++----- >>> 2 files changed, 21 insertions(+), 5 deletions(-) >>> >>> diff --git a/security/integrity/ima/Kconfig >>> b/security/integrity/ima/Kconfig >>> index 60a511c6b583..fc103288852b 100644 >>> --- a/security/integrity/ima/Kconfig >>> +++ b/security/integrity/ima/Kconfig >>> @@ -338,3 +338,14 @@ config IMA_DISABLE_HTABLE >>> default n >>> help >>> This option disables htable to allow measurement of >>> duplicate records. >>> + >>> +config IMA_KEXEC_EXTRA_MEMORY_KB >>> + int >>> + depends on IMA && IMA_KEXEC >>> + default 0 >>> + help >>> + IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be >>> + allocated (in kb) for IMA measurements added during kexec soft >>> reboot. >>> + If set to the default value, an extra half page of memory for >>> + additional measurements will be allocated to maintain backwards >>> + compatibility. >> >> Is there really an issue with 'backwards compatibility' that the user >> needs to know about ? From looking at the code it seems more important >> that a bit of additional memory is reserved now to fit the kexec >> 'load' and 'exec' critical data events but that's not 'backwards >> compatibility'. >> > I was contemplating how to put the right description in place > considering the conversation we had in v3 of this series[1]. > Without that context[1] default 0 could be equally confusing to the end > user. With the phrase 'backwards compatibility', I was trying to > address the potential confusion around the default value 0 in the config > - that it represents half-a-page as default. And that particular value > choice ( half-a-page) is for backwards compatibility. > You are right, I the user shouldn't care about it. But I had to start > somewhere so that we can have this discussion on this thread. :) > > Let me know how this description looks after removing the phrase > 'backwards compatibility': > > " IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be > allocated (in kb) for IMA measurements added during kexec soft reboot. > If set to the default value, an extra half a page of memory for those > additional measurements will be allocated." Sounds good to me. > > Lastly, do you want me to add suggested-by and/or reviewed-by tag to > this patch? Let me know. I will be happy to do so. Either way is fine by me. > > Thanks, > Tushar >
> --- a/security/integrity/ima/ima_kexec.c > +++ b/security/integrity/ima/ima_kexec.c > @@ -121,6 +121,7 @@ void ima_add_kexec_buffer(struct kimage *image) > .buf_min = 0, .buf_max = ULONG_MAX, > .top_down = true }; > unsigned long binary_runtime_size; > + unsigned long extra_size; > > /* use more understandable variable names than defined in kbuf */ > void *kexec_buffer = NULL; > @@ -128,15 +129,19 @@ void ima_add_kexec_buffer(struct kimage *image) > int ret; > > /* > - * Reserve an extra half page of memory for additional measurements > - * added during the kexec load. > + * Reserve extra memory for measurements added during kexec. > */ The memory is still being allocated at kexec "load", so the extra memory is for additional measurement records "since" kexec load. Mimi > - binary_runtime_size = ima_get_binary_runtime_size(); > + if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0) > + extra_size = PAGE_SIZE / 2; > + else > + extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024; > + binary_runtime_size = ima_get_binary_runtime_size() + extra_size; > + > if (binary_runtime_size >= ULONG_MAX - PAGE_SIZE) > kexec_segment_size = ULONG_MAX; > else > - kexec_segment_size = ALIGN(ima_get_binary_runtime_size() + > - PAGE_SIZE / 2, PAGE_SIZE); > + kexec_segment_size = ALIGN(binary_runtime_size, PAGE_SIZE); > + > if ((kexec_segment_size == ULONG_MAX) || > ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) { > pr_err("Binary measurement list too large.\n");
On 1/24/24 06:07, Mimi Zohar wrote: > >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -121,6 +121,7 @@ void ima_add_kexec_buffer(struct kimage *image) >> .buf_min = 0, .buf_max = ULONG_MAX, >> .top_down = true }; >> unsigned long binary_runtime_size; >> + unsigned long extra_size; >> >> /* use more understandable variable names than defined in kbuf */ >> void *kexec_buffer = NULL; >> @@ -128,15 +129,19 @@ void ima_add_kexec_buffer(struct kimage *image) >> int ret; >> >> /* >> - * Reserve an extra half page of memory for additional measurements >> - * added during the kexec load. >> + * Reserve extra memory for measurements added during kexec. >> */ > > The memory is still being allocated at kexec "load", so the extra memory is for > additional measurement records "since" kexec load. > > Mimi > This wording was an attempt to address the comment in v3[1]. So I tried to make the comment generic. But maybe I made it too generic. I will update. [1] Re: [PATCH v3 6/7] ima: configure memory to log events between kexec load and execute https://lore.kernel.org/all/fbe6aa7577875b23a9913a39f858f06f1d2aa903.camel@linux.ibm.com/ "Additional records could be added as a result of the kexec load itself. ... Please remove any references to measurements between kexec load and execute." ~Tushar >> - binary_runtime_size = ima_get_binary_runtime_size(); >> + if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0) >> + extra_size = PAGE_SIZE / 2; >> + else >> + extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024; >> + binary_runtime_size = ima_get_binary_runtime_size() + extra_size; >> + >> if (binary_runtime_size >= ULONG_MAX - PAGE_SIZE) >> kexec_segment_size = ULONG_MAX; >> else >> - kexec_segment_size = ALIGN(ima_get_binary_runtime_size() + >> - PAGE_SIZE / 2, PAGE_SIZE); >> + kexec_segment_size = ALIGN(binary_runtime_size, PAGE_SIZE); >> + >> if ((kexec_segment_size == ULONG_MAX) || >> ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) { >> pr_err("Binary measurement list too large.\n"); >
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index 60a511c6b583..fc103288852b 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -338,3 +338,14 @@ config IMA_DISABLE_HTABLE default n help This option disables htable to allow measurement of duplicate records. + +config IMA_KEXEC_EXTRA_MEMORY_KB + int + depends on IMA && IMA_KEXEC + default 0 + help + IMA_KEXEC_EXTRA_MEMORY_KB determines the extra memory to be + allocated (in kb) for IMA measurements added during kexec soft reboot. + If set to the default value, an extra half page of memory for + additional measurements will be allocated to maintain backwards + compatibility. diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index f26b5b342d84..c126aa6494e9 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -121,6 +121,7 @@ void ima_add_kexec_buffer(struct kimage *image) .buf_min = 0, .buf_max = ULONG_MAX, .top_down = true }; unsigned long binary_runtime_size; + unsigned long extra_size; /* use more understandable variable names than defined in kbuf */ void *kexec_buffer = NULL; @@ -128,15 +129,19 @@ void ima_add_kexec_buffer(struct kimage *image) int ret; /* - * Reserve an extra half page of memory for additional measurements - * added during the kexec load. + * Reserve extra memory for measurements added during kexec. */ - binary_runtime_size = ima_get_binary_runtime_size(); + if (CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB <= 0) + extra_size = PAGE_SIZE / 2; + else + extra_size = CONFIG_IMA_KEXEC_EXTRA_MEMORY_KB * 1024; + binary_runtime_size = ima_get_binary_runtime_size() + extra_size; + if (binary_runtime_size >= ULONG_MAX - PAGE_SIZE) kexec_segment_size = ULONG_MAX; else - kexec_segment_size = ALIGN(ima_get_binary_runtime_size() + - PAGE_SIZE / 2, PAGE_SIZE); + kexec_segment_size = ALIGN(binary_runtime_size, PAGE_SIZE); + if ((kexec_segment_size == ULONG_MAX) || ((kexec_segment_size >> PAGE_SHIFT) > totalram_pages() / 2)) { pr_err("Binary measurement list too large.\n");
The extra memory allocated for carrying the IMA measurement list across kexec is hardcoded as half a PAGE. Make it configurable. Define a Kconfig option, IMA_KEXEC_EXTRA_MEMORY_KB, to configure the extra memory (in kb) to be allocated for IMA measurements added during kexec soft reboot. Ensure the default value of the option is set such that extra half a page of memory for additional measurements is allocated to maintain backwards compatibility. Update ima_add_kexec_buffer() function to allocate memory based on the Kconfig option value, rather than the currently hardcoded one. Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com> --- security/integrity/ima/Kconfig | 11 +++++++++++ security/integrity/ima/ima_kexec.c | 15 ++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-)