| Message ID | 1425456048-16236-5-git-send-email-yinghai@kernel.org |
|---|---|
| State | Not Applicable |
| Headers | show |
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.fleming@intel.com> > Cc: Borislav Petkov <bp@suse.de> > Cc: Kees Cook <keescook@chromium.org> > Cc: Jiri Kosina <jkosina@suse.cz> > Acked-by: Jiri Kosina <jkosina@suse.cz> > Signed-off-by: Yinghai Lu <yinghai@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 >
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,
On Wed, Mar 4, 2015 at 2:16 AM, Borislav Petkov <bp@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-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 4, 2015 at 7:54 AM, Jiri Kosina <jkosina@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-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 4, 2015 at 10:06 AM, Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Mar 4, 2015 at 2:16 AM, Borislav Petkov <bp@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-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Mar 4, 2015 at 7:54 AM, Jiri Kosina <jkosina@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-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Mar 4, 2015 at 2:16 AM, Borislav Petkov <bp@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-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <jkosina@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-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 4, 2015 at 6:58 PM, joeyli <jlee@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-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); + kaslr_enabled = *(data + offset); + early_memunmap(data, offset + 1); } static void __init parse_setup_data(void)
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. Fixes: f47233c2d34f ("x86/mm/ASLR: Propagate base load address calculation") Cc: Matt Fleming <matt.fleming@intel.com> Cc: Borislav Petkov <bp@suse.de> Cc: Kees Cook <keescook@chromium.org> Cc: Jiri Kosina <jkosina@suse.cz> Acked-by: Jiri Kosina <jkosina@suse.cz> Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- arch/x86/kernel/setup.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)