Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Sat, Mar 7, 2015 at 1:05 PM, Borislav Petkov wrote:
> On Fri, Mar 06, 2015 at 11:53:22AM -0800, Yinghai Lu wrote:
> ---
> Commit
>
> f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
>
> started passing KASLR status to kernel proper, but it uses a physical
> address as the vaule, leading to parsing bogus KASLR status in kernel
> proper.
>
> The setup_data linked list and thus the element which contains
> kaslr_enabled is chained together using physical addresses. At the time
> when we access it in the kernel proper, we're already running with
> paging enabled and therefore must access it through its virtual address.
>
> This patch changes the code to use early_memmap() and access the value.
> ---
Good to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 06, 2015 at 11:53:22AM -0800, Yinghai Lu wrote:
> That will get wrong value back for kaslr_enabled in kernel stage.
> 1. When kaslr is not enabled at boot/choose_kernel_location, if kaslr_enabled
> get set wrongly in setup.c, late in module.c::get_module_load_offset
> will return not wanted random module load offset.
> That change behavior when HIBERNATION is defined or nokaslr is passed.
>
> 2. When kaslr is enabled at boot/choose_kernel_location, if kaslr_enabled
> get cleared wrongly in setup.c, late in module.c::get_module_load_offset
> will not return wanted random module load offset.
Now you went from the one extreme to the other. Initially it was
"trivial and obvious" now it is too much unreadable detail which no one
needs.
How about this:
---
Commit
f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
started passing KASLR status to kernel proper, but it uses a physical
address as the vaule, leading to parsing bogus KASLR status in kernel
proper.
The setup_data linked list and thus the element which contains
kaslr_enabled is chained together using physical addresses. At the time
when we access it in the kernel proper, we're already running with
paging enabled and therefore must access it through its virtual address.
This patch changes the code to use early_memmap() and access the value.
---
Complaints?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 06, 2015 at 11:50:54AM -0800, Yinghai Lu wrote: > On Fri, Mar 6, 2015 at 5:33 AM, Borislav Petkov wrote: > > > > > "However, the setup_data linked list and thus the element which contains > > kaslr_enabled is chained together using physical addresses. At the > > time when we access it in the kernel proper, we're already running > > with paging enabled and therefore must access it through its virtual > > address." > > > > That's it, now how hard was to explain it that way? > > No, I don't think your change log is right. > > Actually the old code is using address as value. Am I saying something about using a physical address as value above? Or you can't read now either? -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 06, 2015 at 09:49:25AM -0800, Yinghai Lu wrote: > That is "copy and paste" instead of attachment for easy review. > but gmail web client convert tab to spaces. Next time you send a patch *only* for review *and* *not* for application, do state that at the top like everyone else. Better yet, don't use gmail for sending patches at all. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 06, 2015 at 09:49:25AM -0800, Yinghai Lu wrote: That is copy and paste instead of attachment for easy review. but gmail web client convert tab to spaces. Next time you send a patch *only* for review *and* *not* for application, do state that at the top like everyone else. Better yet, don't use gmail for sending patches at all. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Sat, Mar 7, 2015 at 1:05 PM, Borislav Petkov b...@suse.de wrote: On Fri, Mar 06, 2015 at 11:53:22AM -0800, Yinghai Lu wrote: --- Commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation) started passing KASLR status to kernel proper, but it uses a physical address as the vaule, leading to parsing bogus KASLR status in kernel proper. The setup_data linked list and thus the element which contains kaslr_enabled is chained together using physical addresses. At the time when we access it in the kernel proper, we're already running with paging enabled and therefore must access it through its virtual address. This patch changes the code to use early_memmap() and access the value. --- Good to me. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 06, 2015 at 11:53:22AM -0800, Yinghai Lu wrote: That will get wrong value back for kaslr_enabled in kernel stage. 1. When kaslr is not enabled at boot/choose_kernel_location, if kaslr_enabled get set wrongly in setup.c, late in module.c::get_module_load_offset will return not wanted random module load offset. That change behavior when HIBERNATION is defined or nokaslr is passed. 2. When kaslr is enabled at boot/choose_kernel_location, if kaslr_enabled get cleared wrongly in setup.c, late in module.c::get_module_load_offset will not return wanted random module load offset. Now you went from the one extreme to the other. Initially it was trivial and obvious now it is too much unreadable detail which no one needs. How about this: --- Commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation) started passing KASLR status to kernel proper, but it uses a physical address as the vaule, leading to parsing bogus KASLR status in kernel proper. The setup_data linked list and thus the element which contains kaslr_enabled is chained together using physical addresses. At the time when we access it in the kernel proper, we're already running with paging enabled and therefore must access it through its virtual address. This patch changes the code to use early_memmap() and access the value. --- Complaints? -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 06, 2015 at 11:50:54AM -0800, Yinghai Lu wrote: On Fri, Mar 6, 2015 at 5:33 AM, Borislav Petkov b...@suse.de wrote: However, the setup_data linked list and thus the element which contains kaslr_enabled is chained together using physical addresses. At the time when we access it in the kernel proper, we're already running with paging enabled and therefore must access it through its virtual address. That's it, now how hard was to explain it that way? No, I don't think your change log is right. Actually the old code is using address as value. Am I saying something about using a physical address as value above? Or you can't read now either? -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 6, 2015 at 11:50 AM, Yinghai Lu wrote:
> On Fri, Mar 6, 2015 at 5:33 AM, Borislav Petkov wrote:
>
>>
>> "However, the setup_data linked list and thus the element which contains
>> kaslr_enabled is chained together using physical addresses. At the
>> time when we access it in the kernel proper, we're already running
>> with paging enabled and therefore must access it through its virtual
>> address."
>>
>> That's it, now how hard was to explain it that way?
>
> No, I don't think your change log is right.
>
> Actually the old code is using address as value.
>
> if the old code would be like:
>
> kaslr_enabled = (bool)(*(unsigned char *)(pa_data + sizeof(struct
> setup_data)));
>
> then your change log would be good, but the old code is
>
> kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
Please check if you are ok with this:
Subject: [PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable
commit f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
started passing KASLR status to kernel proper, but it uses address as
the vaule.
That will get wrong value back for kaslr_enabled in kernel stage.
1. When kaslr is not enabled at boot/choose_kernel_location, if kaslr_enabled
get set wrongly in setup.c, late in module.c::get_module_load_offset
will return not wanted random module load offset.
That change behavior when HIBERNATION is defined or nokaslr is passed.
2. When kaslr is enabled at boot/choose_kernel_location, if kaslr_enabled
get cleared wrongly in setup.c, late in module.c::get_module_load_offset
will not return wanted random module load offset.
The setup_data linked list and thus the element which contains
kaslr_enabled is chained together using physical addresses. At the
time when we access it in the kernel proper, we're already running
with paging enabled and therefore must access it through its virtual
address.
This patch changes the code to use early_memmap and access the value,
and will keep boot and kernel consistent with kaslr.
-v3: add checking return from early_memmap according to Boris.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 6, 2015 at 5:33 AM, Borislav Petkov wrote: > > "However, the setup_data linked list and thus the element which contains > kaslr_enabled is chained together using physical addresses. At the > time when we access it in the kernel proper, we're already running > with paging enabled and therefore must access it through its virtual > address." > > That's it, now how hard was to explain it that way? No, I don't think your change log is right. Actually the old code is using address as value. if the old code would be like: kaslr_enabled = (bool)(*(unsigned char *)(pa_data + sizeof(struct setup_data))); then your change log would be good, but the old code is kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data)); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 6, 2015 at 5:33 AM, Borislav Petkov wrote:
> Please use checkpatch before submitting patches:
>
> WARNING: please, no spaces at the start of a line
> #71: FILE: arch/x86/kernel/setup.c:433:
> +unsigned char *data;$
>
> WARNING: please, no spaces at the start of a line
> #72: FILE: arch/x86/kernel/setup.c:434:
> +unsigned long offset = sizeof(struct setup_data);$
>
> WARNING: please, no spaces at the start of a line
> #74: FILE: arch/x86/kernel/setup.c:436:
> +data = early_memremap(pa_data, offset + 1);$
>
> WARNING: please, no spaces at the start of a line
> #75: FILE: arch/x86/kernel/setup.c:437:
> +if (!data) {$
>
> ERROR: code indent should use tabs where possible
> #76: FILE: arch/x86/kernel/setup.c:438:
> +kaslr_enabled = true;$
>
> WARNING: please, no spaces at the start of a line
> #76: FILE: arch/x86/kernel/setup.c:438:
> +kaslr_enabled = true;$
>
> ERROR: code indent should use tabs where possible
> #77: FILE: arch/x86/kernel/setup.c:439:
> +return;$
>
> WARNING: please, no spaces at the start of a line
> #77: FILE: arch/x86/kernel/setup.c:439:
> +return;$
>
> WARNING: please, no spaces at the start of a line
> #78: FILE: arch/x86/kernel/setup.c:440:
> +}$
>
> WARNING: please, no spaces at the start of a line
> #80: FILE: arch/x86/kernel/setup.c:442:
> +kaslr_enabled = *(data + offset);$
>
> WARNING: please, no spaces at the start of a line
> #81: FILE: arch/x86/kernel/setup.c:443:
> +early_memunmap(data, offset + 1);$
>
> total: 2 errors, 9 warnings, 19 lines checked
>
> NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
> scripts/cleanfile
>
> Your patch has style problems, please review.
>
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
That is "copy and paste" instead of attachment for easy review.
but gmail web client convert tab to spaces.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 04, 2015 at 01:32:53PM -0800, Yinghai Lu wrote:
> On Wed, Mar 4, 2015 at 12:00 PM, Ingo Molnar wrote:
> >
> > It is totally unacceptable that you don't do proper analysis of the
> > patches you submit, and that you don't bother writing proper, readable
> > changelogs.
>
> Sorry, please check it again:
>
> Subject: [PATCH v4] x86, kaslr: Get kaslr_enabled back correctly
Subject: x86/kaslr: Access the correct kaslr_enabled variable
> commit f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
> is using address as value for kaslr_enabled.
"commit ... started passing KASLR status to kernel proper."
> That will get wrong value back for kaslr_enabled in kernel stage.
> 1. When kaslr is not enabled at boot/choose_kernel_location, if kaslr_enabled
> get set wrongly in setup.c, late in module.c::get_module_load_offset
> will return not wanted random module load offset.
> That change behavior when HIBERNATION is defined or nokaslr is passed.
>
> 2. When kaslr is enabled at boot/choose_kernel_location, if kaslr_enabled
> get cleared wrongly in setup.c, late in module.c::get_module_load_offset
> will not return wanted random module load offset.
>
> This patch changes the code to use early_memmap and access the value,
> and will keep boot and kernel consistent with kaslr.
Replace all that with:
"However, the setup_data linked list and thus the element which contains
kaslr_enabled is chained together using physical addresses. At the
time when we access it in the kernel proper, we're already running
with paging enabled and therefore must access it through its virtual
address."
That's it, now how hard was to explain it that way?
> -v3: add checking return from early_memmap according to bp.
I guess with "bp" you mean me? You can call me Boris.
> Fixes: f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
> Cc: Matt Fleming
> Cc: Borislav Petkov
> Cc: Kees Cook
> Cc: Jiri Kosina
> Acked-by: Jiri Kosina
> Signed-off-by: Yinghai Lu
>
> ---
> arch/x86/kernel/setup.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> Index: linux-2.6/arch/x86/kernel/setup.c
> ===
> --- linux-2.6.orig/arch/x86/kernel/setup.c
> +++ linux-2.6/arch/x86/kernel/setup.c
> @@ -429,7 +429,18 @@ static void __init reserve_initrd(void)
>
> static void __init parse_kaslr_setup(u64 pa_data, u32 data_len)
> {
> -kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
> +/* kaslr_setup_data is defined in aslr.c */
> +unsigned char *data;
> +unsigned long offset = sizeof(struct setup_data);
> +
> +data = early_memremap(pa_data, offset + 1);
> +if (!data) {
> +kaslr_enabled = true;
> +return;
> +}
> +
> +kaslr_enabled = *(data + offset);
> +early_memunmap(data, offset + 1);
> }
>
> static void __init parse_setup_data(void)
Please use checkpatch before submitting patches:
WARNING: please, no spaces at the start of a line
#71: FILE: arch/x86/kernel/setup.c:433:
+unsigned char *data;$
WARNING: please, no spaces at the start of a line
#72: FILE: arch/x86/kernel/setup.c:434:
+unsigned long offset = sizeof(struct setup_data);$
WARNING: please, no spaces at the start of a line
#74: FILE: arch/x86/kernel/setup.c:436:
+data = early_memremap(pa_data, offset + 1);$
WARNING: please, no spaces at the start of a line
#75: FILE: arch/x86/kernel/setup.c:437:
+if (!data) {$
ERROR: code indent should use tabs where possible
#76: FILE: arch/x86/kernel/setup.c:438:
+kaslr_enabled = true;$
WARNING: please, no spaces at the start of a line
#76: FILE: arch/x86/kernel/setup.c:438:
+kaslr_enabled = true;$
ERROR: code indent should use tabs where possible
#77: FILE: arch/x86/kernel/setup.c:439:
+return;$
WARNING: please, no spaces at the start of a line
#77: FILE: arch/x86/kernel/setup.c:439:
+return;$
WARNING: please, no spaces at the start of a line
#78: FILE: arch/x86/kernel/setup.c:440:
+}$
WARNING: please, no spaces at the start of a line
#80: FILE: arch/x86/kernel/setup.c:442:
+kaslr_enabled = *(data + offset);$
WARNING: please, no spaces at the start of a line
#81: FILE: arch/x86/kernel/setup.c:443:
+early_memunmap(data, offset + 1);$
total: 2 errors, 9 warnings, 19 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile
Your patch has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 04, 2015 at 01:32:53PM -0800, Yinghai Lu wrote:
On Wed, Mar 4, 2015 at 12:00 PM, Ingo Molnar mi...@kernel.org wrote:
It is totally unacceptable that you don't do proper analysis of the
patches you submit, and that you don't bother writing proper, readable
changelogs.
Sorry, please check it again:
Subject: [PATCH v4] x86, kaslr: Get kaslr_enabled back correctly
Subject: x86/kaslr: Access the correct kaslr_enabled variable
commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation)
is using address as value for kaslr_enabled.
commit ... started passing KASLR status to kernel proper.
That will get wrong value back for kaslr_enabled in kernel stage.
1. When kaslr is not enabled at boot/choose_kernel_location, if kaslr_enabled
get set wrongly in setup.c, late in module.c::get_module_load_offset
will return not wanted random module load offset.
That change behavior when HIBERNATION is defined or nokaslr is passed.
2. When kaslr is enabled at boot/choose_kernel_location, if kaslr_enabled
get cleared wrongly in setup.c, late in module.c::get_module_load_offset
will not return wanted random module load offset.
This patch changes the code to use early_memmap and access the value,
and will keep boot and kernel consistent with kaslr.
Replace all that with:
However, the setup_data linked list and thus the element which contains
kaslr_enabled is chained together using physical addresses. At the
time when we access it in the kernel proper, we're already running
with paging enabled and therefore must access it through its virtual
address.
That's it, now how hard was to explain it that way?
-v3: add checking return from early_memmap according to bp.
I guess with bp you mean me? You can call me Boris.
Fixes: f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation)
Cc: Matt Fleming matt.flem...@intel.com
Cc: Borislav Petkov b...@suse.de
Cc: Kees Cook keesc...@chromium.org
Cc: Jiri Kosina jkos...@suse.cz
Acked-by: Jiri Kosina jkos...@suse.cz
Signed-off-by: Yinghai Lu ying...@kernel.org
---
arch/x86/kernel/setup.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
Index: linux-2.6/arch/x86/kernel/setup.c
===
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -429,7 +429,18 @@ static void __init reserve_initrd(void)
static void __init parse_kaslr_setup(u64 pa_data, u32 data_len)
{
-kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
+/* kaslr_setup_data is defined in aslr.c */
+unsigned char *data;
+unsigned long offset = sizeof(struct setup_data);
+
+data = early_memremap(pa_data, offset + 1);
+if (!data) {
+kaslr_enabled = true;
+return;
+}
+
+kaslr_enabled = *(data + offset);
+early_memunmap(data, offset + 1);
}
static void __init parse_setup_data(void)
Please use checkpatch before submitting patches:
WARNING: please, no spaces at the start of a line
#71: FILE: arch/x86/kernel/setup.c:433:
+unsigned char *data;$
WARNING: please, no spaces at the start of a line
#72: FILE: arch/x86/kernel/setup.c:434:
+unsigned long offset = sizeof(struct setup_data);$
WARNING: please, no spaces at the start of a line
#74: FILE: arch/x86/kernel/setup.c:436:
+data = early_memremap(pa_data, offset + 1);$
WARNING: please, no spaces at the start of a line
#75: FILE: arch/x86/kernel/setup.c:437:
+if (!data) {$
ERROR: code indent should use tabs where possible
#76: FILE: arch/x86/kernel/setup.c:438:
+kaslr_enabled = true;$
WARNING: please, no spaces at the start of a line
#76: FILE: arch/x86/kernel/setup.c:438:
+kaslr_enabled = true;$
ERROR: code indent should use tabs where possible
#77: FILE: arch/x86/kernel/setup.c:439:
+return;$
WARNING: please, no spaces at the start of a line
#77: FILE: arch/x86/kernel/setup.c:439:
+return;$
WARNING: please, no spaces at the start of a line
#78: FILE: arch/x86/kernel/setup.c:440:
+}$
WARNING: please, no spaces at the start of a line
#80: FILE: arch/x86/kernel/setup.c:442:
+kaslr_enabled = *(data + offset);$
WARNING: please, no spaces at the start of a line
#81: FILE: arch/x86/kernel/setup.c:443:
+early_memunmap(data, offset + 1);$
total: 2 errors, 9 warnings, 19 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile
Your patch has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 6, 2015 at 5:33 AM, Borislav Petkov b...@suse.de wrote:
Please use checkpatch before submitting patches:
WARNING: please, no spaces at the start of a line
#71: FILE: arch/x86/kernel/setup.c:433:
+unsigned char *data;$
WARNING: please, no spaces at the start of a line
#72: FILE: arch/x86/kernel/setup.c:434:
+unsigned long offset = sizeof(struct setup_data);$
WARNING: please, no spaces at the start of a line
#74: FILE: arch/x86/kernel/setup.c:436:
+data = early_memremap(pa_data, offset + 1);$
WARNING: please, no spaces at the start of a line
#75: FILE: arch/x86/kernel/setup.c:437:
+if (!data) {$
ERROR: code indent should use tabs where possible
#76: FILE: arch/x86/kernel/setup.c:438:
+kaslr_enabled = true;$
WARNING: please, no spaces at the start of a line
#76: FILE: arch/x86/kernel/setup.c:438:
+kaslr_enabled = true;$
ERROR: code indent should use tabs where possible
#77: FILE: arch/x86/kernel/setup.c:439:
+return;$
WARNING: please, no spaces at the start of a line
#77: FILE: arch/x86/kernel/setup.c:439:
+return;$
WARNING: please, no spaces at the start of a line
#78: FILE: arch/x86/kernel/setup.c:440:
+}$
WARNING: please, no spaces at the start of a line
#80: FILE: arch/x86/kernel/setup.c:442:
+kaslr_enabled = *(data + offset);$
WARNING: please, no spaces at the start of a line
#81: FILE: arch/x86/kernel/setup.c:443:
+early_memunmap(data, offset + 1);$
total: 2 errors, 9 warnings, 19 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
scripts/cleanfile
Your patch has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
That is copy and paste instead of attachment for easy review.
but gmail web client convert tab to spaces.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 6, 2015 at 5:33 AM, Borislav Petkov b...@suse.de wrote: However, the setup_data linked list and thus the element which contains kaslr_enabled is chained together using physical addresses. At the time when we access it in the kernel proper, we're already running with paging enabled and therefore must access it through its virtual address. That's it, now how hard was to explain it that way? No, I don't think your change log is right. Actually the old code is using address as value. if the old code would be like: kaslr_enabled = (bool)(*(unsigned char *)(pa_data + sizeof(struct setup_data))); then your change log would be good, but the old code is kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data)); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Fri, Mar 6, 2015 at 11:50 AM, Yinghai Lu ying...@kernel.org wrote: On Fri, Mar 6, 2015 at 5:33 AM, Borislav Petkov b...@suse.de wrote: However, the setup_data linked list and thus the element which contains kaslr_enabled is chained together using physical addresses. At the time when we access it in the kernel proper, we're already running with paging enabled and therefore must access it through its virtual address. That's it, now how hard was to explain it that way? No, I don't think your change log is right. Actually the old code is using address as value. if the old code would be like: kaslr_enabled = (bool)(*(unsigned char *)(pa_data + sizeof(struct setup_data))); then your change log would be good, but the old code is kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data)); Please check if you are ok with this: Subject: [PATCH v4] x86, kaslr: Access the correct kaslr_enabled variable commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation) started passing KASLR status to kernel proper, but it uses address as the vaule. That will get wrong value back for kaslr_enabled in kernel stage. 1. When kaslr is not enabled at boot/choose_kernel_location, if kaslr_enabled get set wrongly in setup.c, late in module.c::get_module_load_offset will return not wanted random module load offset. That change behavior when HIBERNATION is defined or nokaslr is passed. 2. When kaslr is enabled at boot/choose_kernel_location, if kaslr_enabled get cleared wrongly in setup.c, late in module.c::get_module_load_offset will not return wanted random module load offset. The setup_data linked list and thus the element which contains kaslr_enabled is chained together using physical addresses. At the time when we access it in the kernel proper, we're already running with paging enabled and therefore must access it through its virtual address. This patch changes the code to use early_memmap and access the value, and will keep boot and kernel consistent with kaslr. -v3: add checking return from early_memmap according to Boris. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 4, 2015 at 6:58 PM, joeyli wrote: > > After 84c91b7ae merged to v3.17 kernel, hibernate code checks the e280 regions > should not be changed when doing hibernate resume. Without your patch 8, > the hibernate resume checking will randomly fail on the machines that reserved > setup_data in e820 regions. > > Could you please consider to put "[PATCH v2 08/15] x86: Kill > E820_RESERVED_KERN" > to v4.0 kernel? That will trigger SETUP_PCI ioremap warning. That is the reason I want to put it with other setup_data fix. Thanks Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
Hi Yinghai, On Wed, Mar 04, 2015 at 10:12:58AM -0800, Yinghai Lu wrote: > On Wed, Mar 4, 2015 at 7:54 AM, Jiri Kosina wrote: > > > > > Also this 15-patch series needs to be separated into two patchsets. The > > whole series is not appropriate for -rc3, but this particular one at least > > is a regression fix that has to go in. > > The first 4 should go v4.0. > > could leave others to v4.1 > > Thanks > > Yinghai After 84c91b7ae merged to v3.17 kernel, hibernate code checks the e280 regions should not be changed when doing hibernate resume. Without your patch 8, the hibernate resume checking will randomly fail on the machines that reserved setup_data in e820 regions. Could you please consider to put "[PATCH v2 08/15] x86: Kill E820_RESERVED_KERN" to v4.0 kernel? Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 4, 2015 at 12:00 PM, Ingo Molnar wrote:
>
> It is totally unacceptable that you don't do proper analysis of the
> patches you submit, and that you don't bother writing proper, readable
> changelogs.
Sorry, please check it again:
Subject: [PATCH v4] x86, kaslr: Get kaslr_enabled back correctly
commit f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
is using address as value for kaslr_enabled.
That will get wrong value back for kaslr_enabled in kernel stage.
1. When kaslr is not enabled at boot/choose_kernel_location, if kaslr_enabled
get set wrongly in setup.c, late in module.c::get_module_load_offset
will return not wanted random module load offset.
That change behavior when HIBERNATION is defined or nokaslr is passed.
2. When kaslr is enabled at boot/choose_kernel_location, if kaslr_enabled
get cleared wrongly in setup.c, late in module.c::get_module_load_offset
will not return wanted random module load offset.
This patch changes the code to use early_memmap and access the value,
and will keep boot and kernel consistent with kaslr.
-v3: add checking return from early_memmap according to bp.
Fixes: f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
Cc: Matt Fleming
Cc: Borislav Petkov
Cc: Kees Cook
Cc: Jiri Kosina
Acked-by: Jiri Kosina
Signed-off-by: Yinghai Lu
---
arch/x86/kernel/setup.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
Index: linux-2.6/arch/x86/kernel/setup.c
===
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -429,7 +429,18 @@ static void __init reserve_initrd(void)
static void __init parse_kaslr_setup(u64 pa_data, u32 data_len)
{
-kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
+/* kaslr_setup_data is defined in aslr.c */
+unsigned char *data;
+unsigned long offset = sizeof(struct setup_data);
+
+data = early_memremap(pa_data, offset + 1);
+if (!data) {
+kaslr_enabled = true;
+return;
+}
+
+kaslr_enabled = *(data + offset);
+early_memunmap(data, offset + 1);
}
static void __init parse_setup_data(void)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
* Yinghai Lu wrote:
> On Wed, Mar 4, 2015 at 2:16 AM, Borislav Petkov wrote:
> > On Wed, Mar 04, 2015 at 12:00:37AM -0800, Yinghai Lu wrote:
> >> commit f47233c2d34f ("x86/mm/ASLR: Propagate base load address
> >> calculation")
> >> is using address as value for kaslr_enabled.
> >>
> >> That will random kaslr_enabled get that set or cleared.
> >> Will have problem for system really have kaslr enabled.
> >>
> >> -v2: update changelog.
> >
> > This is still not good enough. Please do this:
> >
> > In commit f47233c2d34f we did A. The problem with that is B. Change the
> > code to do C.
> >
> > Now you only have to fill out the A,B and C variables with the
> > respective text which is understandable even for people who don't know
> > this code.
> >
>
> I don't know, that is trivial and obvious.
The fix might be obvious, the effects of the bug are not obvious at
all, as you yourself show that you don't understand your own change,
which is evident from the changelog you've written:
> Please check if it is ok:
>
> Subject: [PATCH v3] x86, kaslr: get kaslr_enabled back correctly
Missing capitalization.
>
> commit f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
> is using address as value for kaslr_enabled.
Missing capitalization. Do you really expect maintainers to fix up
every single sentence of yours??
> That will get wrong value for kaslr_enabled, so have problem for
> system really have kaslr enabled.
This sentence does not parse, nor is it correct: the bug isn't just
triggering on systems that want to have kaslr enabled - but also on
bootloaders that happen to pass in a kaslr boot parameter but have the
switch value disabled...
You also need to point out the important fact that bootloaders that
don't try to use the kaslr extension (i.e. that don't use SETUP_KASLR)
work just fine - this is why the bug was not noticed to begin with,
i.e. the overwhelming majority of systems out there.
> This patch change to using early map and accessing the value.
s/change to using/changes the code to use/
It is totally unacceptable that you don't do proper analysis of the
patches you submit, and that you don't bother writing proper, readable
changelogs.
Your flippant "that is trivial and obvious" attitude towards
changelogs is unacceptable as well. And this is not about English
knowledge: missing capitalization is a very simple concept any
beginning coder should be able to graps the first time it's pointed
out... yet for the past 3 years half of your patches had totally
careless, often unreadable changelogs.
These subpar changelogs and patches show plain laziness, sloppiness
and lack of care to write clean changelogs - and that sloppiness not
only makes it much harder for maintainers to process your patches, but
also tends to creep over into your patches as well, causing repeat
problems again and again...
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
* Yinghai Lu wrote: > On Wed, Mar 4, 2015 at 7:54 AM, Jiri Kosina wrote: > > > > > Also this 15-patch series needs to be separated into two patchsets. The > > whole series is not appropriate for -rc3, but this particular one at least > > is a regression fix that has to go in. > > The first 4 should go v4.0. > > could leave others to v4.1 Then please submit the first 4 only for the time being, and submit the rest once Boris has accepted and applied the fixes. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 4, 2015 at 10:06 AM, Yinghai Lu wrote:
> On Wed, Mar 4, 2015 at 2:16 AM, Borislav Petkov wrote:
>> On Wed, Mar 04, 2015 at 12:00:37AM -0800, Yinghai Lu wrote:
>>> commit f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
>>> is using address as value for kaslr_enabled.
>>>
>>> That will random kaslr_enabled get that set or cleared.
>>> Will have problem for system really have kaslr enabled.
>>>
>>> -v2: update changelog.
>>
>> This is still not good enough. Please do this:
>>
>> In commit f47233c2d34f we did A. The problem with that is B. Change the
>> code to do C.
>>
>> Now you only have to fill out the A,B and C variables with the
>> respective text which is understandable even for people who don't know
>> this code.
Please check if it is ok:
Subject: [PATCH v3] x86, kaslr: get kaslr_enabled back correctly
commit f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
is using address as value for kaslr_enabled.
That will get wrong value for kaslr_enabled, so have problem for system really
have kaslr enabled.
This patch change to using early map and accessing the value.
-v3: add checking about early_memmap according to bp.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 4, 2015 at 7:54 AM, Jiri Kosina wrote: > > Also this 15-patch series needs to be separated into two patchsets. The > whole series is not appropriate for -rc3, but this particular one at least > is a regression fix that has to go in. The first 4 should go v4.0. could leave others to v4.1 Thanks Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 4, 2015 at 2:16 AM, Borislav Petkov wrote:
> On Wed, Mar 04, 2015 at 12:00:37AM -0800, Yinghai Lu wrote:
>> commit f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
>> is using address as value for kaslr_enabled.
>>
>> That will random kaslr_enabled get that set or cleared.
>> Will have problem for system really have kaslr enabled.
>>
>> -v2: update changelog.
>
> This is still not good enough. Please do this:
>
> In commit f47233c2d34f we did A. The problem with that is B. Change the
> code to do C.
>
> Now you only have to fill out the A,B and C variables with the
> respective text which is understandable even for people who don't know
> this code.
>
I don't know, that is trivial and obvious.
the old code use address as value instead of using reference...
>>
>>
>> static void __init parse_kaslr_setup(u64 pa_data, u32 data_len)
>> {
>> - kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
>> + /* kaslr_setup_data is defined in aslr.c */
>> + unsigned char *data;
>> + unsigned long offset = sizeof(struct setup_data);
>> +
>> + data = early_memremap(pa_data, offset + 1);
>
> early_memremap() needs its retval checked before accessing it.
>
will fix that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, 4 Mar 2015, Borislav Petkov wrote:
> On Wed, Mar 04, 2015 at 12:00:37AM -0800, Yinghai Lu wrote:
> > commit f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
> > is using address as value for kaslr_enabled.
> >
> > That will random kaslr_enabled get that set or cleared.
> > Will have problem for system really have kaslr enabled.
> >
> > -v2: update changelog.
>
> This is still not good enough. Please do this:
>
> In commit f47233c2d34f we did A. The problem with that is B. Change the
> code to do C.
>
> Now you only have to fill out the A,B and C variables with the
> respective text which is understandable even for people who don't know
> this code.
Also this 15-patch series needs to be separated into two patchsets. The
whole series is not appropriate for -rc3, but this particular one at least
is a regression fix that has to go in.
Thanks,
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 04, 2015 at 12:00:37AM -0800, Yinghai Lu wrote:
> commit f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
> is using address as value for kaslr_enabled.
>
> That will random kaslr_enabled get that set or cleared.
> Will have problem for system really have kaslr enabled.
>
> -v2: update changelog.
This is still not good enough. Please do this:
In commit f47233c2d34f we did A. The problem with that is B. Change the
code to do C.
Now you only have to fill out the A,B and C variables with the
respective text which is understandable even for people who don't know
this code.
>
> Fixes: f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation")
> Cc: Matt Fleming
> Cc: Borislav Petkov
> Cc: Kees Cook
> Cc: Jiri Kosina
> Acked-by: Jiri Kosina
> Signed-off-by: Yinghai Lu
> ---
> arch/x86/kernel/setup.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 98dc931..05d444f 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -429,7 +429,13 @@ static void __init reserve_initrd(void)
>
> static void __init parse_kaslr_setup(u64 pa_data, u32 data_len)
> {
> - kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
> + /* kaslr_setup_data is defined in aslr.c */
> + unsigned char *data;
> + unsigned long offset = sizeof(struct setup_data);
> +
> + data = early_memremap(pa_data, offset + 1);
early_memremap() needs its retval checked before accessing it.
> + kaslr_enabled = *(data + offset);
> + early_memunmap(data, offset + 1);
> }
>
> static void __init parse_setup_data(void)
> --
> 1.8.4.5
>
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, 4 Mar 2015, Borislav Petkov wrote: On Wed, Mar 04, 2015 at 12:00:37AM -0800, Yinghai Lu wrote: commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation) is using address as value for kaslr_enabled. That will random kaslr_enabled get that set or cleared. Will have problem for system really have kaslr enabled. -v2: update changelog. This is still not good enough. Please do this: In commit f47233c2d34f we did A. The problem with that is B. Change the code to do C. Now you only have to fill out the A,B and C variables with the respective text which is understandable even for people who don't know this code. Also this 15-patch series needs to be separated into two patchsets. The whole series is not appropriate for -rc3, but this particular one at least is a regression fix that has to go in. Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 4, 2015 at 2:16 AM, Borislav Petkov b...@alien8.de wrote:
On Wed, Mar 04, 2015 at 12:00:37AM -0800, Yinghai Lu wrote:
commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation)
is using address as value for kaslr_enabled.
That will random kaslr_enabled get that set or cleared.
Will have problem for system really have kaslr enabled.
-v2: update changelog.
This is still not good enough. Please do this:
In commit f47233c2d34f we did A. The problem with that is B. Change the
code to do C.
Now you only have to fill out the A,B and C variables with the
respective text which is understandable even for people who don't know
this code.
I don't know, that is trivial and obvious.
the old code use address as value instead of using reference...
static void __init parse_kaslr_setup(u64 pa_data, u32 data_len)
{
- kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
+ /* kaslr_setup_data is defined in aslr.c */
+ unsigned char *data;
+ unsigned long offset = sizeof(struct setup_data);
+
+ data = early_memremap(pa_data, offset + 1);
early_memremap() needs its retval checked before accessing it.
will fix that.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 4, 2015 at 7:54 AM, Jiri Kosina jkos...@suse.cz wrote: Also this 15-patch series needs to be separated into two patchsets. The whole series is not appropriate for -rc3, but this particular one at least is a regression fix that has to go in. The first 4 should go v4.0. could leave others to v4.1 Thanks Yinghai -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 4, 2015 at 10:06 AM, Yinghai Lu ying...@kernel.org wrote: On Wed, Mar 4, 2015 at 2:16 AM, Borislav Petkov b...@alien8.de wrote: On Wed, Mar 04, 2015 at 12:00:37AM -0800, Yinghai Lu wrote: commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation) is using address as value for kaslr_enabled. That will random kaslr_enabled get that set or cleared. Will have problem for system really have kaslr enabled. -v2: update changelog. This is still not good enough. Please do this: In commit f47233c2d34f we did A. The problem with that is B. Change the code to do C. Now you only have to fill out the A,B and C variables with the respective text which is understandable even for people who don't know this code. Please check if it is ok: Subject: [PATCH v3] x86, kaslr: get kaslr_enabled back correctly commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation) is using address as value for kaslr_enabled. That will get wrong value for kaslr_enabled, so have problem for system really have kaslr enabled. This patch change to using early map and accessing the value. -v3: add checking about early_memmap according to bp. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 4, 2015 at 12:00 PM, Ingo Molnar mi...@kernel.org wrote:
It is totally unacceptable that you don't do proper analysis of the
patches you submit, and that you don't bother writing proper, readable
changelogs.
Sorry, please check it again:
Subject: [PATCH v4] x86, kaslr: Get kaslr_enabled back correctly
commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation)
is using address as value for kaslr_enabled.
That will get wrong value back for kaslr_enabled in kernel stage.
1. When kaslr is not enabled at boot/choose_kernel_location, if kaslr_enabled
get set wrongly in setup.c, late in module.c::get_module_load_offset
will return not wanted random module load offset.
That change behavior when HIBERNATION is defined or nokaslr is passed.
2. When kaslr is enabled at boot/choose_kernel_location, if kaslr_enabled
get cleared wrongly in setup.c, late in module.c::get_module_load_offset
will not return wanted random module load offset.
This patch changes the code to use early_memmap and access the value,
and will keep boot and kernel consistent with kaslr.
-v3: add checking return from early_memmap according to bp.
Fixes: f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation)
Cc: Matt Fleming matt.flem...@intel.com
Cc: Borislav Petkov b...@suse.de
Cc: Kees Cook keesc...@chromium.org
Cc: Jiri Kosina jkos...@suse.cz
Acked-by: Jiri Kosina jkos...@suse.cz
Signed-off-by: Yinghai Lu ying...@kernel.org
---
arch/x86/kernel/setup.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
Index: linux-2.6/arch/x86/kernel/setup.c
===
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -429,7 +429,18 @@ static void __init reserve_initrd(void)
static void __init parse_kaslr_setup(u64 pa_data, u32 data_len)
{
-kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
+/* kaslr_setup_data is defined in aslr.c */
+unsigned char *data;
+unsigned long offset = sizeof(struct setup_data);
+
+data = early_memremap(pa_data, offset + 1);
+if (!data) {
+kaslr_enabled = true;
+return;
+}
+
+kaslr_enabled = *(data + offset);
+early_memunmap(data, offset + 1);
}
static void __init parse_setup_data(void)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
* Yinghai Lu ying...@kernel.org wrote: On Wed, Mar 4, 2015 at 2:16 AM, Borislav Petkov b...@alien8.de wrote: On Wed, Mar 04, 2015 at 12:00:37AM -0800, Yinghai Lu wrote: commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation) is using address as value for kaslr_enabled. That will random kaslr_enabled get that set or cleared. Will have problem for system really have kaslr enabled. -v2: update changelog. This is still not good enough. Please do this: In commit f47233c2d34f we did A. The problem with that is B. Change the code to do C. Now you only have to fill out the A,B and C variables with the respective text which is understandable even for people who don't know this code. I don't know, that is trivial and obvious. The fix might be obvious, the effects of the bug are not obvious at all, as you yourself show that you don't understand your own change, which is evident from the changelog you've written: Please check if it is ok: Subject: [PATCH v3] x86, kaslr: get kaslr_enabled back correctly Missing capitalization. commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation) is using address as value for kaslr_enabled. Missing capitalization. Do you really expect maintainers to fix up every single sentence of yours?? That will get wrong value for kaslr_enabled, so have problem for system really have kaslr enabled. This sentence does not parse, nor is it correct: the bug isn't just triggering on systems that want to have kaslr enabled - but also on bootloaders that happen to pass in a kaslr boot parameter but have the switch value disabled... You also need to point out the important fact that bootloaders that don't try to use the kaslr extension (i.e. that don't use SETUP_KASLR) work just fine - this is why the bug was not noticed to begin with, i.e. the overwhelming majority of systems out there. This patch change to using early map and accessing the value. s/change to using/changes the code to use/ It is totally unacceptable that you don't do proper analysis of the patches you submit, and that you don't bother writing proper, readable changelogs. Your flippant that is trivial and obvious attitude towards changelogs is unacceptable as well. And this is not about English knowledge: missing capitalization is a very simple concept any beginning coder should be able to graps the first time it's pointed out... yet for the past 3 years half of your patches had totally careless, often unreadable changelogs. These subpar changelogs and patches show plain laziness, sloppiness and lack of care to write clean changelogs - and that sloppiness not only makes it much harder for maintainers to process your patches, but also tends to creep over into your patches as well, causing repeat problems again and again... Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
* Yinghai Lu ying...@kernel.org wrote: On Wed, Mar 4, 2015 at 7:54 AM, Jiri Kosina jkos...@suse.cz wrote: Also this 15-patch series needs to be separated into two patchsets. The whole series is not appropriate for -rc3, but this particular one at least is a regression fix that has to go in. The first 4 should go v4.0. could leave others to v4.1 Then please submit the first 4 only for the time being, and submit the rest once Boris has accepted and applied the fixes. Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
Hi Yinghai, On Wed, Mar 04, 2015 at 10:12:58AM -0800, Yinghai Lu wrote: On Wed, Mar 4, 2015 at 7:54 AM, Jiri Kosina jkos...@suse.cz wrote: Also this 15-patch series needs to be separated into two patchsets. The whole series is not appropriate for -rc3, but this particular one at least is a regression fix that has to go in. The first 4 should go v4.0. could leave others to v4.1 Thanks Yinghai After 84c91b7ae merged to v3.17 kernel, hibernate code checks the e280 regions should not be changed when doing hibernate resume. Without your patch 8, the hibernate resume checking will randomly fail on the machines that reserved setup_data in e820 regions. Could you please consider to put [PATCH v2 08/15] x86: Kill E820_RESERVED_KERN to v4.0 kernel? Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 4, 2015 at 6:58 PM, joeyli j...@suse.com wrote: After 84c91b7ae merged to v3.17 kernel, hibernate code checks the e280 regions should not be changed when doing hibernate resume. Without your patch 8, the hibernate resume checking will randomly fail on the machines that reserved setup_data in e820 regions. Could you please consider to put [PATCH v2 08/15] x86: Kill E820_RESERVED_KERN to v4.0 kernel? That will trigger SETUP_PCI ioremap warning. That is the reason I want to put it with other setup_data fix. Thanks Yinghai -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 04/15] x86, kaslr: get kaslr_enabled back correctly
On Wed, Mar 04, 2015 at 12:00:37AM -0800, Yinghai Lu wrote:
commit f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation)
is using address as value for kaslr_enabled.
That will random kaslr_enabled get that set or cleared.
Will have problem for system really have kaslr enabled.
-v2: update changelog.
This is still not good enough. Please do this:
In commit f47233c2d34f we did A. The problem with that is B. Change the
code to do C.
Now you only have to fill out the A,B and C variables with the
respective text which is understandable even for people who don't know
this code.
Fixes: f47233c2d34f (x86/mm/ASLR: Propagate base load address calculation)
Cc: Matt Fleming matt.flem...@intel.com
Cc: Borislav Petkov b...@suse.de
Cc: Kees Cook keesc...@chromium.org
Cc: Jiri Kosina jkos...@suse.cz
Acked-by: Jiri Kosina jkos...@suse.cz
Signed-off-by: Yinghai Lu ying...@kernel.org
---
arch/x86/kernel/setup.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 98dc931..05d444f 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -429,7 +429,13 @@ static void __init reserve_initrd(void)
static void __init parse_kaslr_setup(u64 pa_data, u32 data_len)
{
- kaslr_enabled = (bool)(pa_data + sizeof(struct setup_data));
+ /* kaslr_setup_data is defined in aslr.c */
+ unsigned char *data;
+ unsigned long offset = sizeof(struct setup_data);
+
+ data = early_memremap(pa_data, offset + 1);
early_memremap() needs its retval checked before accessing it.
+ kaslr_enabled = *(data + offset);
+ early_memunmap(data, offset + 1);
}
static void __init parse_setup_data(void)
--
1.8.4.5
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
