* [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
@ 2024-08-05 15:07 ` Petr Tesarik
0 siblings, 0 replies; 16+ messages in thread
From: Petr Tesarik @ 2024-08-05 15:07 UTC (permalink / raw)
To: Eric Biederman, Sourabh Jain, Hari Bathini, Baoquan He,
Andrew Morton, Eric DeVolder, open list:KEXEC, open list
Cc: Petr Tesarik, stable
From: Petr Tesarik <ptesarik@suse.com>
Fix the condition to exclude the elfcorehdr segment from the SHA digest
calculation.
The j iterator is an index into the output sha_regions[] array, not into
the input image->segment[] array. Once it reaches image->elfcorehdr_index,
all subsequent segments are excluded. Besides, if the purgatory segment
precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
the calculation.
Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
Cc: stable@kernel.org
Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
kernel/kexec_file.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 3d64290d24c9..3eedb8c226ad 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
#ifdef CONFIG_CRASH_HOTPLUG
/* Exclude elfcorehdr segment to allow future changes via hotplug */
- if (j == image->elfcorehdr_index)
+ if (i == image->elfcorehdr_index)
continue;
#endif
--
2.45.2
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
2024-08-05 15:07 ` Petr Tesarik
@ 2024-08-05 15:17 ` Petr Tesarik
-1 siblings, 0 replies; 16+ messages in thread
From: Petr Tesarik @ 2024-08-05 15:17 UTC (permalink / raw)
To: Petr Tesarik
Cc: Eric Biederman, Sourabh Jain, Hari Bathini, Baoquan He,
Andrew Morton, Eric DeVolder, open list:KEXEC, open list, stable
+Cc Eric DeVolder (current email address)
On Mon, 5 Aug 2024 17:07:50 +0200
Petr Tesarik <petr.tesarik@suse.com> wrote:
> From: Petr Tesarik <ptesarik@suse.com>
>
> Fix the condition to exclude the elfcorehdr segment from the SHA digest
> calculation.
>
> The j iterator is an index into the output sha_regions[] array, not into
> the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> all subsequent segments are excluded. Besides, if the purgatory segment
> precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> the calculation.
>
> Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
> Cc: stable@kernel.org
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
> kernel/kexec_file.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 3d64290d24c9..3eedb8c226ad 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
>
> #ifdef CONFIG_CRASH_HOTPLUG
> /* Exclude elfcorehdr segment to allow future changes via hotplug */
> - if (j == image->elfcorehdr_index)
> + if (i == image->elfcorehdr_index)
> continue;
> #endif
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
@ 2024-08-05 15:17 ` Petr Tesarik
0 siblings, 0 replies; 16+ messages in thread
From: Petr Tesarik @ 2024-08-05 15:17 UTC (permalink / raw)
To: Petr Tesarik
Cc: Eric Biederman, Sourabh Jain, Hari Bathini, Baoquan He,
Andrew Morton, Eric DeVolder, open list:KEXEC, open list, stable
+Cc Eric DeVolder (current email address)
On Mon, 5 Aug 2024 17:07:50 +0200
Petr Tesarik <petr.tesarik@suse.com> wrote:
> From: Petr Tesarik <ptesarik@suse.com>
>
> Fix the condition to exclude the elfcorehdr segment from the SHA digest
> calculation.
>
> The j iterator is an index into the output sha_regions[] array, not into
> the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> all subsequent segments are excluded. Besides, if the purgatory segment
> precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> the calculation.
>
> Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
> Cc: stable@kernel.org
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
> kernel/kexec_file.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 3d64290d24c9..3eedb8c226ad 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
>
> #ifdef CONFIG_CRASH_HOTPLUG
> /* Exclude elfcorehdr segment to allow future changes via hotplug */
> - if (j == image->elfcorehdr_index)
> + if (i == image->elfcorehdr_index)
> continue;
> #endif
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
2024-08-05 15:07 ` Petr Tesarik
@ 2024-08-05 22:59 ` Baoquan He
-1 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2024-08-05 22:59 UTC (permalink / raw)
To: Petr Tesarik
Cc: Eric Biederman, Sourabh Jain, Hari Bathini, Andrew Morton,
Eric DeVolder, open list:KEXEC, open list, Petr Tesarik, stable
On 08/05/24 at 05:07pm, Petr Tesarik wrote:
> From: Petr Tesarik <ptesarik@suse.com>
>
> Fix the condition to exclude the elfcorehdr segment from the SHA digest
> calculation.
>
> The j iterator is an index into the output sha_regions[] array, not into
> the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> all subsequent segments are excluded. Besides, if the purgatory segment
> precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> the calculation.
Indeed, good catch.
Acked-by: Baoquan He <bhe@redhat.com>
>
> Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
> Cc: stable@kernel.org
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
> kernel/kexec_file.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 3d64290d24c9..3eedb8c226ad 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
>
> #ifdef CONFIG_CRASH_HOTPLUG
> /* Exclude elfcorehdr segment to allow future changes via hotplug */
> - if (j == image->elfcorehdr_index)
> + if (i == image->elfcorehdr_index)
> continue;
> #endif
>
> --
> 2.45.2
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
@ 2024-08-05 22:59 ` Baoquan He
0 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2024-08-05 22:59 UTC (permalink / raw)
To: Petr Tesarik
Cc: Eric Biederman, Sourabh Jain, Hari Bathini, Andrew Morton,
Eric DeVolder, open list:KEXEC, open list, Petr Tesarik, stable
On 08/05/24 at 05:07pm, Petr Tesarik wrote:
> From: Petr Tesarik <ptesarik@suse.com>
>
> Fix the condition to exclude the elfcorehdr segment from the SHA digest
> calculation.
>
> The j iterator is an index into the output sha_regions[] array, not into
> the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> all subsequent segments are excluded. Besides, if the purgatory segment
> precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> the calculation.
Indeed, good catch.
Acked-by: Baoquan He <bhe@redhat.com>
>
> Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
> Cc: stable@kernel.org
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
> kernel/kexec_file.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 3d64290d24c9..3eedb8c226ad 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
>
> #ifdef CONFIG_CRASH_HOTPLUG
> /* Exclude elfcorehdr segment to allow future changes via hotplug */
> - if (j == image->elfcorehdr_index)
> + if (i == image->elfcorehdr_index)
> continue;
> #endif
>
> --
> 2.45.2
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
2024-08-05 22:59 ` Baoquan He
@ 2024-08-16 6:33 ` Petr Tesarik
-1 siblings, 0 replies; 16+ messages in thread
From: Petr Tesarik @ 2024-08-16 6:33 UTC (permalink / raw)
To: Baoquan He
Cc: Petr Tesarik, Eric Biederman, Sourabh Jain, Hari Bathini,
Andrew Morton, Eric DeVolder, open list:KEXEC, open list, stable
On Tue, 6 Aug 2024 06:59:35 +0800
Baoquan He <bhe@redhat.com> wrote:
> On 08/05/24 at 05:07pm, Petr Tesarik wrote:
> > From: Petr Tesarik <ptesarik@suse.com>
> >
> > Fix the condition to exclude the elfcorehdr segment from the SHA digest
> > calculation.
> >
> > The j iterator is an index into the output sha_regions[] array, not into
> > the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> > all subsequent segments are excluded. Besides, if the purgatory segment
> > precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> > the calculation.
>
> Indeed, good catch.
>
> Acked-by: Baoquan He <bhe@redhat.com>
Thank you, Baoquan.
Who should apply the fix now? How can it be merged into Linus tree?
Petr T
> >
> > Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
> > Cc: stable@kernel.org
> > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> > ---
> > kernel/kexec_file.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index 3d64290d24c9..3eedb8c226ad 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
> >
> > #ifdef CONFIG_CRASH_HOTPLUG
> > /* Exclude elfcorehdr segment to allow future changes via hotplug */
> > - if (j == image->elfcorehdr_index)
> > + if (i == image->elfcorehdr_index)
> > continue;
> > #endif
> >
> > --
> > 2.45.2
> >
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
@ 2024-08-16 6:33 ` Petr Tesarik
0 siblings, 0 replies; 16+ messages in thread
From: Petr Tesarik @ 2024-08-16 6:33 UTC (permalink / raw)
To: Baoquan He
Cc: Petr Tesarik, Eric Biederman, Sourabh Jain, Hari Bathini,
Andrew Morton, Eric DeVolder, open list:KEXEC, open list, stable
On Tue, 6 Aug 2024 06:59:35 +0800
Baoquan He <bhe@redhat.com> wrote:
> On 08/05/24 at 05:07pm, Petr Tesarik wrote:
> > From: Petr Tesarik <ptesarik@suse.com>
> >
> > Fix the condition to exclude the elfcorehdr segment from the SHA digest
> > calculation.
> >
> > The j iterator is an index into the output sha_regions[] array, not into
> > the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> > all subsequent segments are excluded. Besides, if the purgatory segment
> > precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> > the calculation.
>
> Indeed, good catch.
>
> Acked-by: Baoquan He <bhe@redhat.com>
Thank you, Baoquan.
Who should apply the fix now? How can it be merged into Linus tree?
Petr T
> >
> > Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
> > Cc: stable@kernel.org
> > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> > ---
> > kernel/kexec_file.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index 3d64290d24c9..3eedb8c226ad 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
> >
> > #ifdef CONFIG_CRASH_HOTPLUG
> > /* Exclude elfcorehdr segment to allow future changes via hotplug */
> > - if (j == image->elfcorehdr_index)
> > + if (i == image->elfcorehdr_index)
> > continue;
> > #endif
> >
> > --
> > 2.45.2
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
2024-08-05 15:07 ` Petr Tesarik
@ 2024-08-16 7:12 ` Baoquan He
-1 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2024-08-16 7:12 UTC (permalink / raw)
To: Andrew Morton, Petr Tesarik
Cc: Eric Biederman, Sourabh Jain, Hari Bathini, Eric DeVolder,
open list:KEXEC, open list, Petr Tesarik, stable
Hi Andrew,
On 08/05/24 at 05:07pm, Petr Tesarik wrote:
> From: Petr Tesarik <ptesarik@suse.com>
>
> Fix the condition to exclude the elfcorehdr segment from the SHA digest
> calculation.
>
> The j iterator is an index into the output sha_regions[] array, not into
> the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> all subsequent segments are excluded. Besides, if the purgatory segment
> precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> the calculation.
Ping.
This is a good fix, could you pick this one into your tree?
Thanks
Baoquan
>
> Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
> Cc: stable@kernel.org
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
> kernel/kexec_file.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 3d64290d24c9..3eedb8c226ad 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
>
> #ifdef CONFIG_CRASH_HOTPLUG
> /* Exclude elfcorehdr segment to allow future changes via hotplug */
> - if (j == image->elfcorehdr_index)
> + if (i == image->elfcorehdr_index)
> continue;
> #endif
>
> --
> 2.45.2
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
@ 2024-08-16 7:12 ` Baoquan He
0 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2024-08-16 7:12 UTC (permalink / raw)
To: Andrew Morton, Petr Tesarik
Cc: Eric Biederman, Sourabh Jain, Hari Bathini, Eric DeVolder,
open list:KEXEC, open list, Petr Tesarik, stable
Hi Andrew,
On 08/05/24 at 05:07pm, Petr Tesarik wrote:
> From: Petr Tesarik <ptesarik@suse.com>
>
> Fix the condition to exclude the elfcorehdr segment from the SHA digest
> calculation.
>
> The j iterator is an index into the output sha_regions[] array, not into
> the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> all subsequent segments are excluded. Besides, if the purgatory segment
> precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> the calculation.
Ping.
This is a good fix, could you pick this one into your tree?
Thanks
Baoquan
>
> Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
> Cc: stable@kernel.org
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
> kernel/kexec_file.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 3d64290d24c9..3eedb8c226ad 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
>
> #ifdef CONFIG_CRASH_HOTPLUG
> /* Exclude elfcorehdr segment to allow future changes via hotplug */
> - if (j == image->elfcorehdr_index)
> + if (i == image->elfcorehdr_index)
> continue;
> #endif
>
> --
> 2.45.2
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
2024-08-05 15:07 ` Petr Tesarik
@ 2024-08-16 12:54 ` Eric W. Biederman
-1 siblings, 0 replies; 16+ messages in thread
From: Eric W. Biederman @ 2024-08-16 12:54 UTC (permalink / raw)
To: Petr Tesarik
Cc: Sourabh Jain, Hari Bathini, Baoquan He, Andrew Morton,
Eric DeVolder, open list:KEXEC, open list, Petr Tesarik, stable
Petr Tesarik <petr.tesarik@suse.com> writes:
> From: Petr Tesarik <ptesarik@suse.com>
>
> Fix the condition to exclude the elfcorehdr segment from the SHA digest
> calculation.
>
> The j iterator is an index into the output sha_regions[] array, not into
> the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> all subsequent segments are excluded. Besides, if the purgatory segment
> precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> the calculation.
I would rather make CONFIG_CRASH_HOTPLUG depend on broken.
The hash is supposed to include everything we depend upon so when
a borken machine corrupts something we can detect that corruption
and not attempt to take a crash dump.
The elfcorehdr is definitely something that needs to be part of the
hash.
So please go back to the drawing board and find a way to include the
program header in the hash even with CONFIG_CRASH_HOTPLUG.
Eric
> Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
> Cc: stable@kernel.org
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
> kernel/kexec_file.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 3d64290d24c9..3eedb8c226ad 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
>
> #ifdef CONFIG_CRASH_HOTPLUG
> /* Exclude elfcorehdr segment to allow future changes via hotplug */
> - if (j == image->elfcorehdr_index)
> + if (i == image->elfcorehdr_index)
> continue;
> #endif
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
@ 2024-08-16 12:54 ` Eric W. Biederman
0 siblings, 0 replies; 16+ messages in thread
From: Eric W. Biederman @ 2024-08-16 12:54 UTC (permalink / raw)
To: Petr Tesarik
Cc: Sourabh Jain, Hari Bathini, Baoquan He, Andrew Morton,
Eric DeVolder, open list:KEXEC, open list, Petr Tesarik, stable
Petr Tesarik <petr.tesarik@suse.com> writes:
> From: Petr Tesarik <ptesarik@suse.com>
>
> Fix the condition to exclude the elfcorehdr segment from the SHA digest
> calculation.
>
> The j iterator is an index into the output sha_regions[] array, not into
> the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> all subsequent segments are excluded. Besides, if the purgatory segment
> precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> the calculation.
I would rather make CONFIG_CRASH_HOTPLUG depend on broken.
The hash is supposed to include everything we depend upon so when
a borken machine corrupts something we can detect that corruption
and not attempt to take a crash dump.
The elfcorehdr is definitely something that needs to be part of the
hash.
So please go back to the drawing board and find a way to include the
program header in the hash even with CONFIG_CRASH_HOTPLUG.
Eric
> Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
> Cc: stable@kernel.org
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
> kernel/kexec_file.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 3d64290d24c9..3eedb8c226ad 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
>
> #ifdef CONFIG_CRASH_HOTPLUG
> /* Exclude elfcorehdr segment to allow future changes via hotplug */
> - if (j == image->elfcorehdr_index)
> + if (i == image->elfcorehdr_index)
> continue;
> #endif
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
2024-08-16 12:54 ` Eric W. Biederman
@ 2024-08-16 13:42 ` Petr Tesarik
-1 siblings, 0 replies; 16+ messages in thread
From: Petr Tesarik @ 2024-08-16 13:42 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Sourabh Jain, Hari Bathini, Baoquan He, Andrew Morton,
Eric DeVolder, open list:KEXEC, open list, stable
On Fri, 16 Aug 2024 07:54:52 -0500
"Eric W. Biederman" <ebiederm@xmission.com> wrote:
> Petr Tesarik <petr.tesarik@suse.com> writes:
>
> > From: Petr Tesarik <ptesarik@suse.com>
> >
> > Fix the condition to exclude the elfcorehdr segment from the SHA digest
> > calculation.
> >
> > The j iterator is an index into the output sha_regions[] array, not into
> > the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> > all subsequent segments are excluded. Besides, if the purgatory segment
> > precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> > the calculation.
>
> I would rather make CONFIG_CRASH_HOTPLUG depend on broken.
>
> The hash is supposed to include everything we depend upon so when
> a borken machine corrupts something we can detect that corruption
> and not attempt to take a crash dump.
>
> The elfcorehdr is definitely something that needs to be part of the
> hash.
>
> So please go back to the drawing board and find a way to include the
> program header in the hash even with CONFIG_CRASH_HOTPLUG.
I'm not trying to argue with your opinion, but it seems you're
complaining to the wrong person. My present patch merely fixes an
obvious trivial mistake in commit f7cc804a9fd4 ("kexec: exclude
elfcorehdr from the segment digest") to exclude _only_ the elfcorehdr
segment from the hash (which was intended) and not any _other_ segments
(which was not intended but is what currently happens).
If you want to change the direction of kexec hotplug support, feel free
to revert commit f7cc804a9fd4 instead. That would also fix the bug and
make me happy.
Petr T
> Eric
>
>
> > Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
> > Cc: stable@kernel.org
> > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> > ---
> > kernel/kexec_file.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index 3d64290d24c9..3eedb8c226ad 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
> >
> > #ifdef CONFIG_CRASH_HOTPLUG
> > /* Exclude elfcorehdr segment to allow future changes via hotplug */
> > - if (j == image->elfcorehdr_index)
> > + if (i == image->elfcorehdr_index)
> > continue;
> > #endif
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
@ 2024-08-16 13:42 ` Petr Tesarik
0 siblings, 0 replies; 16+ messages in thread
From: Petr Tesarik @ 2024-08-16 13:42 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Sourabh Jain, Hari Bathini, Baoquan He, Andrew Morton,
Eric DeVolder, open list:KEXEC, open list, stable
On Fri, 16 Aug 2024 07:54:52 -0500
"Eric W. Biederman" <ebiederm@xmission.com> wrote:
> Petr Tesarik <petr.tesarik@suse.com> writes:
>
> > From: Petr Tesarik <ptesarik@suse.com>
> >
> > Fix the condition to exclude the elfcorehdr segment from the SHA digest
> > calculation.
> >
> > The j iterator is an index into the output sha_regions[] array, not into
> > the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> > all subsequent segments are excluded. Besides, if the purgatory segment
> > precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> > the calculation.
>
> I would rather make CONFIG_CRASH_HOTPLUG depend on broken.
>
> The hash is supposed to include everything we depend upon so when
> a borken machine corrupts something we can detect that corruption
> and not attempt to take a crash dump.
>
> The elfcorehdr is definitely something that needs to be part of the
> hash.
>
> So please go back to the drawing board and find a way to include the
> program header in the hash even with CONFIG_CRASH_HOTPLUG.
I'm not trying to argue with your opinion, but it seems you're
complaining to the wrong person. My present patch merely fixes an
obvious trivial mistake in commit f7cc804a9fd4 ("kexec: exclude
elfcorehdr from the segment digest") to exclude _only_ the elfcorehdr
segment from the hash (which was intended) and not any _other_ segments
(which was not intended but is what currently happens).
If you want to change the direction of kexec hotplug support, feel free
to revert commit f7cc804a9fd4 instead. That would also fix the bug and
make me happy.
Petr T
> Eric
>
>
> > Fixes: f7cc804a9fd4 ("kexec: exclude elfcorehdr from the segment digest")
> > Cc: stable@kernel.org
> > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> > ---
> > kernel/kexec_file.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index 3d64290d24c9..3eedb8c226ad 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -752,7 +752,7 @@ static int kexec_calculate_store_digests(struct kimage *image)
> >
> > #ifdef CONFIG_CRASH_HOTPLUG
> > /* Exclude elfcorehdr segment to allow future changes via hotplug */
> > - if (j == image->elfcorehdr_index)
> > + if (i == image->elfcorehdr_index)
> > continue;
> > #endif
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
2024-08-16 12:54 ` Eric W. Biederman
@ 2024-09-12 9:54 ` Baoquan He
-1 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2024-09-12 9:54 UTC (permalink / raw)
To: Eric W. Biederman, Sourabh Jain
Cc: Petr Tesarik, Hari Bathini, Andrew Morton, Eric DeVolder,
open list:KEXEC, open list, Petr Tesarik, stable
Hi Eric,
On 08/16/24 at 07:54am, Eric W. Biederman wrote:
> Petr Tesarik <petr.tesarik@suse.com> writes:
>
> > From: Petr Tesarik <ptesarik@suse.com>
> >
> > Fix the condition to exclude the elfcorehdr segment from the SHA digest
> > calculation.
> >
> > The j iterator is an index into the output sha_regions[] array, not into
> > the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> > all subsequent segments are excluded. Besides, if the purgatory segment
> > precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> > the calculation.
>
> I would rather make CONFIG_CRASH_HOTPLUG depend on broken.
>
> The hash is supposed to include everything we depend upon so when
> a borken machine corrupts something we can detect that corruption
> and not attempt to take a crash dump.
>
> The elfcorehdr is definitely something that needs to be part of the
> hash.
>
> So please go back to the drawing board and find a way to include the
> program header in the hash even with CONFIG_CRASH_HOTPLUG.
Thanks for checking this and adding your advice, and sorry for late
reply.
It's me who suggested Eric DeVolder not adding elfcorehdr into kdump
kernel iamge hash during reviewing his patch. I need explain this if
people has concern. When I suggested this, what I considered are:
1) The code change will be much simpler. As you can see, later Eric
DeVolder's patchset experienced rounds of reviewing and finally
merged. Below is his final round:
- [PATCH v28 0/8] crash: Kernel handling of CPU and memory hot un/plug
2) The efficiency will be improved very much relative to adding
elfcorehdr to the entire hash. When cpu/mem hotplug triggered,
we only touch elfcorehdr area, but don't need access the entire
loading segments.
3) The elfcorehdr size is very tiny relative to kernel image and initrd.
E.g on x86, it's less than 1M, which is tiny relative to dozens of
kernel image and initrd.
Surely, adding all loading segments into hash is the best. While
attracted by above benefits, I tend to not add for the time being. I am
open to this, if anyone has concern about the security and is interested
in the adding as a kernel project practice in the future, it's welcomed.
Here I'd like to request comment from Sourabh since he and other IBM dev
added the support to ppc too. Different than generic ARCH, IBM dev can
be seen as a end user, maybe we can hear how they evaluate the balance
between the risk and benefit.
Thanks
Baoquan
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] kexec_file: fix elfcorehdr digest exclusion when CONFIG_CRASH_HOTPLUG=y
@ 2024-09-12 9:54 ` Baoquan He
0 siblings, 0 replies; 16+ messages in thread
From: Baoquan He @ 2024-09-12 9:54 UTC (permalink / raw)
To: Eric W. Biederman, Sourabh Jain
Cc: Petr Tesarik, Hari Bathini, Andrew Morton, Eric DeVolder,
open list:KEXEC, open list, Petr Tesarik, stable
Hi Eric,
On 08/16/24 at 07:54am, Eric W. Biederman wrote:
> Petr Tesarik <petr.tesarik@suse.com> writes:
>
> > From: Petr Tesarik <ptesarik@suse.com>
> >
> > Fix the condition to exclude the elfcorehdr segment from the SHA digest
> > calculation.
> >
> > The j iterator is an index into the output sha_regions[] array, not into
> > the input image->segment[] array. Once it reaches image->elfcorehdr_index,
> > all subsequent segments are excluded. Besides, if the purgatory segment
> > precedes the elfcorehdr segment, the elfcorehdr may be wrongly included in
> > the calculation.
>
> I would rather make CONFIG_CRASH_HOTPLUG depend on broken.
>
> The hash is supposed to include everything we depend upon so when
> a borken machine corrupts something we can detect that corruption
> and not attempt to take a crash dump.
>
> The elfcorehdr is definitely something that needs to be part of the
> hash.
>
> So please go back to the drawing board and find a way to include the
> program header in the hash even with CONFIG_CRASH_HOTPLUG.
Thanks for checking this and adding your advice, and sorry for late
reply.
It's me who suggested Eric DeVolder not adding elfcorehdr into kdump
kernel iamge hash during reviewing his patch. I need explain this if
people has concern. When I suggested this, what I considered are:
1) The code change will be much simpler. As you can see, later Eric
DeVolder's patchset experienced rounds of reviewing and finally
merged. Below is his final round:
- [PATCH v28 0/8] crash: Kernel handling of CPU and memory hot un/plug
2) The efficiency will be improved very much relative to adding
elfcorehdr to the entire hash. When cpu/mem hotplug triggered,
we only touch elfcorehdr area, but don't need access the entire
loading segments.
3) The elfcorehdr size is very tiny relative to kernel image and initrd.
E.g on x86, it's less than 1M, which is tiny relative to dozens of
kernel image and initrd.
Surely, adding all loading segments into hash is the best. While
attracted by above benefits, I tend to not add for the time being. I am
open to this, if anyone has concern about the security and is interested
in the adding as a kernel project practice in the future, it's welcomed.
Here I'd like to request comment from Sourabh since he and other IBM dev
added the support to ppc too. Different than generic ARCH, IBM dev can
be seen as a end user, maybe we can hear how they evaluate the balance
between the risk and benefit.
Thanks
Baoquan
^ permalink raw reply [flat|nested] 16+ messages in thread