kexec ppc64: fix misaligned cmdline
Subsystems (2)
PATCH 1/1 kexec ppc64: fix misaligned cmdline
If the cmdline changes between boots, we can get misalignment of the bootargs entry, which in turn corrupts our device tree blob and hence kills our kexec boot.
Specifically, if the cmdline length was >= 8 before and the new cmdline length is < 8, we can get corruption.
--- kexec/arch/ppc64/fs2dt.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
kexec/arch/ppc64/fs2dt.c +6 -1
@@ -197,6 +197,7 @@ static void putprops(char *fn, struct distruct dirent *dp;int i =0 , fd, len;struct stat statbuf; +int dt_realigned =0 ;for (i =0 ; i < numlist; i++) { dp = nlist[i];
@@ -243,8 +244,10 @@ static void putprops(char *fn, struct di *dt++ = len; *dt++ =propnum (fn); -if ((len >=8 ) && ((unsigned long )dt &0x4 )) +if ((len >=8 ) && ((unsigned long )dt &0x4 )){ dt++; + dt_realigned =1 ; + } fd =open (pathname,O_RDONLY );if (fd ==-1 )
@@ -283,6 +286,8 @@ static void putprops(char *fn, struct distrcat (local_cmdline," " ); cmd_len =strlen (local_cmdline); cmd_len = cmd_len +1 ; +if (dt_realigned && cmd_len <8 ) + dt--;memcpy (dt, local_cmdline,cmd_len); len = cmd_len; *dt_len = cmd_len;
Quoted 3 lines · Michael Neuling
If the cmdline changes between boots, we can get misalignment of the bootargs entry, which in turn corrupts our device tree blob and hence kills our kexec boot.
...
Quoted 9 lines
kexec/arch/ppc64/fs2dt.c +3
-1-if ((len >=8 ) && ((unsigned long )dt &0x4 )) +if ((len >=8 ) && ((unsigned long )dt &0x4 )){ dt++; + dt_realigned =1 ; + } fd =open (pathname,O_RDONLY );if (fd ==-1 )
kexec/arch/ppc64/fs2dt.c static void putprops(char *fn, struct di · line 291
Quoted 6 lines
kexec/arch/ppc64/fs2dt.c +2
-0@@ -283,6 +286,8 @@ static void putprops(char *fn, struct distrcat (local_cmdline," " ); cmd_len =strlen (local_cmdline); cmd_len = cmd_len +1 ; +if (dt_realigned && cmd_len <8 ) + dt--;
While this appears to fix the stated problem, did you explore my suggestion of deleting and creating the property like we do for the initrd base and size?
Deleting and recreating should also handle the case of no boot-args in the original /chosen sub-tree (when the bootloader didn't (need to) supply a command line).
milton
Quoted 3 lines
While this appears to fix the stated problem, did you explore my suggestion of deleting and creating the property like we do for the initrd base and size?
Yeah, but it didn't seem "hapazard" enough for kexec. :-)
Quoted 3 lines
Deleting and recreating should also handle the case of no boot-args in the original /chosen sub-tree (when the bootloader didn't (need to) supply a command line).
Yeah, we're screwed when the first boot has no bootargs.
Let me get the policy right. We blow away the old command line except for the "root=blah" item. Correct?
Mikey
root= cmdline modification in kexec (was Re: [PATCH] kexec ppc64: fix misaligned cmdline )
Quoted 6 lines
Quoted 3 lines
While this appears to fix the stated problem, did you explore my suggestion of deleting and creating the property like we do for the initrd base and size?
Yeah, but it didn't seem "hapazard" enough for kexec. :-)
Quoted 8 lines
Quoted 3 lines
Deleting and recreating should also handle the case of no boot-args in the original /chosen sub-tree (when the bootloader didn't (need to) supply a command line).
Yeah, we're screwed when the first boot has no bootargs.
Let me get the policy right. We blow away the old command line except for the "root=blah" item. Correct?
OK Looks like we parse the new command line to see if there is a 'root=blah' parameter. If yes, use that, otherwise we use what was on the last boot.
Is this what other architectures are doing? Is there any other special cases we need to look out for?
Mikey
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Re: root= cmdline modification in kexec (was Re: [PATCH] kexec ppc64: fix misaligned cmdline )
Quoted 3 lines · Michael Neuling
OK Looks like we parse the new command line to see if there is a 'root=blah' parameter. If yes, use that, otherwise we use what was on the last boot.
For PPC64, if the user does not pass the kernel parameter, it is taken from the existing kernel parameter. In this case "crashkernel=X@Y" parameter is removed. If user explicitly gives the parameter, it is used.
Quoted 3 lines
Is this what other architectures are doing? Is there any other special cases we need to look out for?
I am not sure about x86, I hope they expect that user has to _pass_ the kernel parameter. They don't copy the kernel parameter from current kernel.
Quoted 7 lines
Mikey
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
PATCH 1/1 kexec ppc64: fix misaligned cmdline
In reply to Michael NeulingIf the cmdline changes between boots, we can get misalignment of the bootargs entry, which in turn corrupts our device tree blob and hence kills our kexec boot. Also, if there was no /chosen/bootargs previously, we'd never add a new one.
This ignores the bootargs while parsing the tree and inserts it later.
---
This hopefully address issues raised by Milton.
kexec/arch/ppc64/fs2dt.c | 95 +++++++++++++++++++++++++++++++---------------- 1 file changed, 63 insertions(+), 32 deletions(-)
kexec/arch/ppc64/fs2dt.c +63 -32
@@ -18,6 +18,7 @@* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. */ +#define _GNU_SOURCE#include <sys/types.h> #include <sys/stat.h>
@@ -233,6 +234,12 @@ static void putprops(char *fn, struct di !reuse_initrd)continue ; +/* This property will be created later in putnode() So +* ignore it now. +*/ +if (!strcmp (dp->d_name,"bootargs" )) +continue ; +if (!S_ISREG (statbuf.st_mode))continue ;
@@ -257,38 +264,6 @@ static void putprops(char *fn, struct dicheckprop (fn, dt, len); -/* Get the cmdline from the device-tree and modify it */ -if (!strcmp (dp->d_name,"bootargs" )) { -int cmd_len; -char temp_cmdline[COMMAND_LINE_SIZE ] = {"" }; -char *param =NULL ; - cmd_len =strlen (local_cmdline); -if (cmd_len !=0 ) { - param =strstr (local_cmdline,"crashkernel=" ); -if (param) - crash_param =1 ; - param =strstr (local_cmdline,"root=" ); - } -if (!param) { -char *old_param; -memcpy (temp_cmdline, dt, len); - param =strstr (temp_cmdline,"root=" ); -if (param) { - old_param =strtok (param," " ); -if (cmd_len !=0 ) -strcat (local_cmdline," " ); -strcat (local_cmdline, old_param); - } - } -strcat (local_cmdline," " ); - cmd_len =strlen (local_cmdline); - cmd_len = cmd_len +1 ; -memcpy (dt, local_cmdline,cmd_len); - len = cmd_len; - *dt_len = cmd_len; -fprintf (stderr,"Modified cmdline:%s\n" , local_cmdline); - } - dt += (len +3 )/4 ;if (!strcmp (dp->d_name,"reg" ) && usablemem_rgns.size)add_usable_mem_property (fd, len);
@@ -385,6 +360,62 @@ static void putnode(void)reserve (initrd_base, initrd_size); } +/* Add cmdline to the second kernel. Check to see if the new +* cmdline has a root=. If not, use the old root= cmdline. */ +if (!strcmp (basename,"/chosen/" )) { +size_t cmd_len =0 ; +char *param =NULL ; + + cmd_len =strlen (local_cmdline); +if (cmd_len !=0 ) { + param =strstr (local_cmdline,"crashkernel=" ); +if (param) + crash_param =1 ; +/* does the new cmdline have a root= ? ... */ + param =strstr (local_cmdline,"root=" ); + } + +/* ... if not, grab root= from the old command line */ +if (!param) { +char filename[MAXPATH ]; +FILE *fp; +char *last_cmdline =NULL ; +char *old_param; + +strcpy (filename, pathname); +strcat (filename,"bootargs" ); + fp =fopen (filename,"r" ); +if (fp) { +if (getline (&last_cmdline, &cmd_len, fp) ==-1 ) +die ("unable to read %s\n" , filename); + + param =strstr (last_cmdline,"root=" ); +if (param) { + old_param =strtok (param," " ); +if (cmd_len !=0 ) +strcat (local_cmdline," " ); +strcat (local_cmdline, old_param); + } + } +if (last_cmdline) +free (last_cmdline); + } +strcat (local_cmdline," " ); + cmd_len =strlen (local_cmdline); + cmd_len = cmd_len +1 ; + +/* add new bootargs */ + *dt++ =3 ; + *dt++ = cmd_len; + *dt++ =propnum ("bootargs" ); +if ((cmd_len >=8 ) && ((unsigned long )dt &0x4 )) + dt++; +memcpy (dt, local_cmdline,cmd_len); + dt += (cmd_len +3 )/4 ; + +fprintf (stderr,"Modified cmdline:%s\n" , local_cmdline); + } +for (i=0 ; i < numlist; i++) { dp = namelist[i];strcpy (dn, dp->d_name);
Quoted 8 lines · Michael Neuling
If the cmdline changes between boots, we can get misalignment of the bootargs entry, which in turn corrupts our device tree blob and hence kills our kexec boot. Also, if there was no /chosen/bootargs previously, we'd never add a new one.
This ignores the bootargs while parsing the tree and inserts it later.
Signed-off-by Michael Neuling <[email protected]>
Hi.
I tested this with PS3 and it works as expected. I added it to my ps3-kexec-tools.git. With this patch mainline kexec will work with the PS3.
I still have a work-in-progress patch started by David Woodhouse to support building a 32 bit kexec executable for PPC64 platforms. I don't have any definite plan to work on it though.
-Geoff
Horms
AppliedQuoted 8 lines · Michael Neuling
If the cmdline changes between boots, we can get misalignment of the bootargs entry, which in turn corrupts our device tree blob and hence kills our kexec boot. Also, if there was no /chosen/bootargs previously, we'd never add a new one.
This ignores the bootargs while parsing the tree and inserts it later.
Signed-off-by Michael Neuling <[email protected]>
Thanks, applied.
Signature · 3 lines
Horms H: http://www.vergenet.net/~horms/ W: http://www.valinux.co.jp/en/
Quoted 6 lines · Miichael Neuling
If the cmdline changes between boots, we can get misalignment of the bootargs entry, which in turn corrupts our device tree blob and hence kills our kexec boot.
Specifically, if the cmdline length was >= 8 before and the new cmdline length is < 8, we can get corruption.
Hrm. Have you considered using dtc for this conversion, rather than this somewhat dubious looking fs2dt?
Signature · 4 lines
David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Quoted 9 lines
Quoted 6 lines
If the cmdline changes between boots, we can get misalignment of the bootargs entry, which in turn corrupts our device tree blob and hence kills our kexec boot.
Specifically, if the cmdline length was >= 8 before and the new cmdline length is < 8, we can get corruption.
Hrm. Have you considered using dtc for this conversion, rather than this somewhat dubious looking fs2dt?
Numerous times.
fs2dt needs to be converted to use the device tree library, but that will take a lot more work than a 5 minute patch.
Mikey
Quoted 7 lines · Miichael Neuling
Quoted 7 lines
Quoted 6 lines
If the cmdline changes between boots, we can get misalignment of the bootargs entry, which in turn corrupts our device tree blob and hence kills our kexec boot.
Specifically, if the cmdline length was >= 8 before and the new cmdline length is < 8, we can get corruption.
Quoted 7 lines
Quoted 2 lines
Hrm. Have you considered using dtc for this conversion, rather than this somewhat dubious looking fs2dt?
Numerous times.
fs2dt needs to be converted to use the device tree library, but that will take a lot more work than a 5 minute patch.
Well, sure.. but do you need fs2dt at all. dtc has an "fs" input mode..
Signature · 4 lines
David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Quoted 11 lines · Miichael Neuling
Quoted 10 lines
Quoted 9 lines
Quoted 6 lines
If the cmdline changes between boots, we can get misalignment of the bootargs entry, which in turn corrupts our device tree blob and hence kills our kexec boot.
Specifically, if the cmdline length was >= 8 before and the new cmdline length is < 8, we can get corruption.
Hrm. Have you considered using dtc for this conversion, rather than this somewhat dubious looking fs2dt?
Quoted 8 lines
Quoted 4 lines
Numerous times.
fs2dt needs to be converted to use the device tree library, but that will take a lot more work than a 5 minute patch.
Well, sure.. but do you need fs2dt at all. dtc has an "fs" input mode..
As discussed offline, you can do this with the --devicetreeblob kexec option. The issue is the special cases like having to modify the kernel cmdline options or initrd locations.
Mikey