hw/s390x/ipl: Fix crash that occurs when -kernel is used with small images
PATCH 1/1 hw/s390x/ipl: Fix crash that occurs when -kernel is used with small images
Add a sanity check to fix the following crash:
$ echo "Insane in the mainframe" > /tmp/test.txt $ s390x-softmmu/qemu-system-s390x -nographic -kernel /tmp/test.txt Segmentation fault (core dumped)
--- hw/s390x/ipl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
hw/s390x/ipl.c +2 -1
@@ -168,7 +168,8 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)* we can not rely on the ELF entry point - it was 0x800 (the SALIPL * loader) and it won't work. For this case we force it to 0x10000, too. */ -if (pentry ==KERN_IMAGE_START || pentry ==0x800 ) { +if ((pentry ==KERN_IMAGE_START || pentry ==0x800 ) && + kernel_size >KERN_PARM_AREA +strlen (ipl->cmdline)) { ipl->start_addr =KERN_IMAGE_START ;/* Overwrite parameters in the kernel image, which are "rom" */ strcpy (rom_ptr (KERN_PARM_AREA ), ipl->cmdline);
Signature · 1 line
1.8.3.1
Quoted 7 lines · Thomas Huth
Add a sanity check to fix the following crash:
$ echo "Insane in the mainframe" > /tmp/test.txt $ s390x-softmmu/qemu-system-s390x -nographic -kernel /tmp/test.txt Segmentation fault (core dumped)Signed-off-by Thomas Huth <[email protected]>
I think a similar problem exists for INITRD_PARM_START and INITRD_PARM_SIZE. No?
Quoted 19 lines
--- hw/s390x/ipl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)hw/s390x/ipl.c +2
-1@@ -168,7 +168,8 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)* we can not rely on the ELF entry point - it was 0x800 (the SALIPL * loader) and it won't work. For this case we force it to 0x10000, too. */ -if (pentry ==KERN_IMAGE_START || pentry ==0x800 ) { +if ((pentry ==KERN_IMAGE_START || pentry ==0x800 ) && + kernel_size >KERN_PARM_AREA +strlen (ipl->cmdline)) { ipl->start_addr =KERN_IMAGE_START ;/* Overwrite parameters in the kernel image, which are "rom" */ strcpy (rom_ptr (KERN_PARM_AREA ), ipl->cmdline);
Quoted 7 lines · On Mon, 11 Jun 2018 09:49:39 +0200 Christian Borntraeger…
Quoted 6 lines · Thomas Huth
Add a sanity check to fix the following crash:
$ echo "Insane in the mainframe" > /tmp/test.txt $ s390x-softmmu/qemu-system-s390x -nographic -kernel /tmp/test.txt Segmentation fault (core dumped)
Quoted 5 lines
Quoted 1 line
Signed-off-by Thomas Huth <[email protected]>Reviewed-by Christian Borntraeger <[email protected]>I think a similar problem exists for
INITRD_PARM_STARTandINITRD_PARM_SIZE. No?
I think so as well.
Quoted 6 lines
Quoted 6 lines
--- hw/s390x/ipl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 16 lines
Quoted 6 lines
hw/s390x/ipl.c +0
-0hw/s390x/ipl.c static void s390_ipl_realize(DeviceState *dev, Error **errp) · line 175
Quoted 9 lines
hw/s390x/ipl.c +2
-1@@ -168,7 +168,8 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)* we can not rely on the ELF entry point - it was 0x800 (the SALIPL * loader) and it won't work. For this case we force it to 0x10000, too. */ -if (pentry ==KERN_IMAGE_START || pentry ==0x800 ) { +if ((pentry ==KERN_IMAGE_START || pentry ==0x800 ) && + kernel_size >KERN_PARM_AREA +strlen (ipl->cmdline)) { ipl->start_addr =KERN_IMAGE_START ;/* Overwrite parameters in the kernel image, which are "rom" */ strcpy (rom_ptr (KERN_PARM_AREA ), ipl->cmdline);
The outcome of this is that we don't write into areas we must not write into, but we still have a broken "kernel" and will simply fail if the thing we're pointing to isn't a valid PSW. I guess that's what we want ("crap in, crap out"), i.e. no fallback to the bios or something like that?
Quoted 10 lines · Cornelia Huck
Quoted 7 lines · On Mon, 11 Jun 2018 09:49:39 +0200 Christian Borntraeger…
Quoted 6 lines · Thomas Huth
Add a sanity check to fix the following crash:
$ echo "Insane in the mainframe" > /tmp/test.txt $ s390x-softmmu/qemu-system-s390x -nographic -kernel /tmp/test.txt Segmentation fault (core dumped)
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Thomas Huth <[email protected]>Reviewed-by Christian Borntraeger <[email protected]>I think a similar problem exists for
INITRD_PARM_STARTandINITRD_PARM_SIZE. No?I think so as well.
You're right:
$ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt \
-initrd /tmp/test.txt
Segmentation fault (core dumped)Shall I sent a v2 of this patch, or do you prefer a separate patch for that issue?
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
--- hw/s390x/ipl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)hw/s390x/ipl.c +2
-1@@ -168,7 +168,8 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)* we can not rely on the ELF entry point - it was 0x800 (the SALIPL * loader) and it won't work. For this case we force it to 0x10000, too. */ -if (pentry ==KERN_IMAGE_START || pentry ==0x800 ) { +if ((pentry ==KERN_IMAGE_START || pentry ==0x800 ) && + kernel_size >KERN_PARM_AREA +strlen (ipl->cmdline)) { ipl->start_addr =KERN_IMAGE_START ;
Quoted 8 lines
Quoted 2 lines
hw/s390x/ipl.c static void s390_ipl_realize(DeviceState *dev, Error **errp) · line 175
Quoted 2 lines
hw/s390x/ipl.c +0
-0/* Overwrite parameters in the kernel image, which are "rom" */ strcpy (rom_ptr (KERN_PARM_AREA ), ipl->cmdline);The outcome of this is that we don't write into areas we must not write into, but we still have a broken "kernel" and will simply fail if the thing we're pointing to isn't a valid PSW. I guess that's what we want ("crap in, crap out"), i.e. no fallback to the bios or something like that?
Yes, I think "crap in, crap out" is ok here. Theoretically, the user could also have a self-made micro-kernel that is just one byte smaller than KERN_PARM_AREA, and this would still work with this patch, so no need for an extra error message in that case.
Thomas
Quoted 19 lines · Thomas Huth
Quoted 10 lines · Cornelia Huck
Quoted 7 lines · On Mon, 11 Jun 2018 09:49:39 +0200 Christian Borntraeger…
Quoted 6 lines · Thomas Huth
Add a sanity check to fix the following crash:
$ echo "Insane in the mainframe" > /tmp/test.txt $ s390x-softmmu/qemu-system-s390x -nographic -kernel /tmp/test.txt Segmentation fault (core dumped)Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Thomas Huth <[email protected]>Reviewed-by Christian Borntraeger <[email protected]>I think a similar problem exists for
INITRD_PARM_STARTandINITRD_PARM_SIZE. No?I think so as well.
Quoted 8 lines
You're right:
$ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt \ -initrd /tmp/test.txt Segmentation fault (core dumped)Shall I sent a v2 of this patch, or do you prefer a separate patch for that issue?
Whatever is easier for you.
Quoted 30 lines
Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
--- hw/s390x/ipl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)hw/s390x/ipl.c +2
-1@@ -168,7 +168,8 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)* we can not rely on the ELF entry point - it was 0x800 (the SALIPL * loader) and it won't work. For this case we force it to 0x10000, too. */ -if (pentry ==KERN_IMAGE_START || pentry ==0x800 ) { +if ((pentry ==KERN_IMAGE_START || pentry ==0x800 ) && + kernel_size >KERN_PARM_AREA +strlen (ipl->cmdline)) { ipl->start_addr =KERN_IMAGE_START ;Quoted 8 lines
Quoted 2 lines
hw/s390x/ipl.c static void s390_ipl_realize(DeviceState *dev, Error **errp) · line 175
Quoted 2 lines
hw/s390x/ipl.c +0
-0/* Overwrite parameters in the kernel image, which are "rom" */ strcpy (rom_ptr (KERN_PARM_AREA ), ipl->cmdline);The outcome of this is that we don't write into areas we must not write into, but we still have a broken "kernel" and will simply fail if the thing we're pointing to isn't a valid PSW. I guess that's what we want ("crap in, crap out"), i.e. no fallback to the bios or something like that?
Yes, I think "crap in, crap out" is ok here. Theoretically, the user could also have a self-made micro-kernel that is just one byte smaller than
KERN_PARM_AREA, and this would still work with this patch, so no need for an extra error message in that case.