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 <thuth@redhat.com>
---
hw/s390x/ipl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 04245b5..9bb9b50 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -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);
--
1.8.3.1
On 06/10/2018 03:12 PM, Thomas Huth wrote: > 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 <thuth@redhat.com> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> I think a similar problem exists for INITRD_PARM_START and INITRD_PARM_SIZE. No? > --- > hw/s390x/ipl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 04245b5..9bb9b50 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -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); >
On Mon, 11 Jun 2018 09:49:39 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 06/10/2018 03:12 PM, Thomas Huth wrote: > > 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 <thuth@redhat.com> > > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > > I think a similar problem exists for INITRD_PARM_START and INITRD_PARM_SIZE. No? I think so as well. > > > > --- > > hw/s390x/ipl.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > > index 04245b5..9bb9b50 100644 > > --- a/hw/s390x/ipl.c > > +++ b/hw/s390x/ipl.c > > @@ -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?
On 11.06.2018 11:24, Cornelia Huck wrote: > On Mon, 11 Jun 2018 09:49:39 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 06/10/2018 03:12 PM, Thomas Huth wrote: >>> 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 <thuth@redhat.com> >> >> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >> >> I think a similar problem exists for INITRD_PARM_START and INITRD_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? >>> --- >>> hw/s390x/ipl.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >>> index 04245b5..9bb9b50 100644 >>> --- a/hw/s390x/ipl.c >>> +++ b/hw/s390x/ipl.c >>> @@ -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? 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
On 06/11/2018 12:08 PM, Thomas Huth wrote: > On 11.06.2018 11:24, Cornelia Huck wrote: >> On Mon, 11 Jun 2018 09:49:39 +0200 >> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >> >>> On 06/10/2018 03:12 PM, Thomas Huth wrote: >>>> 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 <thuth@redhat.com> >>> >>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> >>> I think a similar problem exists for INITRD_PARM_START and INITRD_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? Whatever is easier for you. > >>>> --- >>>> hw/s390x/ipl.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >>>> index 04245b5..9bb9b50 100644 >>>> --- a/hw/s390x/ipl.c >>>> +++ b/hw/s390x/ipl.c >>>> @@ -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? > > 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.
© 2016 - 2024 Red Hat, Inc.