* [Qemu-devel] [PATCH v2] hw/s390x/ipl: Fix crashes that occurs when -kernel is used with small images
@ 2018-06-11 17:26 Thomas Huth
2018-06-12 7:37 ` [Qemu-devel] [qemu-s390x] " Christian Borntraeger
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Thomas Huth @ 2018-06-11 17:26 UTC (permalink / raw)
To: Christian Borntraeger, qemu-s390x; +Cc: Cornelia Huck, qemu-devel
Add a sanity checks to fix the following two crashes:
$ echo "Insane in the mainframe" > /tmp/test.txt
$ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt -append xyz
Segmentation fault (core dumped)
$ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt -initrd /tmp/test.txt
Segmentation fault (core dumped)
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
v2: Fixed 2nd crash with -initrd
hw/s390x/ipl.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 04245b5..2ff8f20 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);
@@ -195,8 +196,10 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
* we have to overwrite values in the kernel image,
* which are "rom"
*/
- stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
- stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
+ if (kernel_size > INITRD_PARM_SIZE + 8) {
+ stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
+ stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
+ }
}
}
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [qemu-s390x] [PATCH v2] hw/s390x/ipl: Fix crashes that occurs when -kernel is used with small images
2018-06-11 17:26 [Qemu-devel] [PATCH v2] hw/s390x/ipl: Fix crashes that occurs when -kernel is used with small images Thomas Huth
@ 2018-06-12 7:37 ` Christian Borntraeger
2018-06-12 9:26 ` [Qemu-devel] " Cornelia Huck
2018-06-14 19:08 ` [Qemu-devel] [qemu-s390x] " Thomas Huth
2 siblings, 0 replies; 6+ messages in thread
From: Christian Borntraeger @ 2018-06-12 7:37 UTC (permalink / raw)
To: Thomas Huth, qemu-s390x; +Cc: Cornelia Huck, qemu-devel
On 06/11/2018 07:26 PM, Thomas Huth wrote:
> Add a sanity checks to fix the following two crashes:
>
> $ echo "Insane in the mainframe" > /tmp/test.txt
> $ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt -append xyz
> Segmentation fault (core dumped)
> $ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt -initrd /tmp/test.txt
> Segmentation fault (core dumped)
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> v2: Fixed 2nd crash with -initrd
>
> hw/s390x/ipl.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 04245b5..2ff8f20 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);
> @@ -195,8 +196,10 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
> * we have to overwrite values in the kernel image,
> * which are "rom"
> */
> - stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
> - stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
> + if (kernel_size > INITRD_PARM_SIZE + 8) {
> + stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
> + stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
> + }
> }
> }
> /*
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2] hw/s390x/ipl: Fix crashes that occurs when -kernel is used with small images
2018-06-11 17:26 [Qemu-devel] [PATCH v2] hw/s390x/ipl: Fix crashes that occurs when -kernel is used with small images Thomas Huth
2018-06-12 7:37 ` [Qemu-devel] [qemu-s390x] " Christian Borntraeger
@ 2018-06-12 9:26 ` Cornelia Huck
2018-06-14 19:08 ` [Qemu-devel] [qemu-s390x] " Thomas Huth
2 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2018-06-12 9:26 UTC (permalink / raw)
To: Thomas Huth; +Cc: Christian Borntraeger, qemu-s390x, qemu-devel
On Mon, 11 Jun 2018 19:26:47 +0200
Thomas Huth <thuth@redhat.com> wrote:
I've tweaked $SUBJECT to
hw/s390x/ipl: Fix crashes when using -kernel/-initrd with small images
> Add a sanity checks to fix the following two crashes:
Also, s/Add a sanity checks/Add sanity checks/
>
> $ echo "Insane in the mainframe" > /tmp/test.txt
> $ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt -append xyz
> Segmentation fault (core dumped)
> $ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt -initrd /tmp/test.txt
> Segmentation fault (core dumped)
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> v2: Fixed 2nd crash with -initrd
>
> hw/s390x/ipl.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
Thanks, applied.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [qemu-s390x] [PATCH v2] hw/s390x/ipl: Fix crashes that occurs when -kernel is used with small images
2018-06-11 17:26 [Qemu-devel] [PATCH v2] hw/s390x/ipl: Fix crashes that occurs when -kernel is used with small images Thomas Huth
2018-06-12 7:37 ` [Qemu-devel] [qemu-s390x] " Christian Borntraeger
2018-06-12 9:26 ` [Qemu-devel] " Cornelia Huck
@ 2018-06-14 19:08 ` Thomas Huth
2018-06-15 7:13 ` Cornelia Huck
2 siblings, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2018-06-14 19:08 UTC (permalink / raw)
To: Christian Borntraeger, qemu-s390x; +Cc: Cornelia Huck, qemu-devel
On 11.06.2018 19:26, Thomas Huth wrote:
> Add a sanity checks to fix the following two crashes:
>
> $ echo "Insane in the mainframe" > /tmp/test.txt
> $ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt -append xyz
> Segmentation fault (core dumped)
> $ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt -initrd /tmp/test.txt
> Segmentation fault (core dumped)
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> v2: Fixed 2nd crash with -initrd
>
> hw/s390x/ipl.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 04245b5..2ff8f20 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);
> @@ -195,8 +196,10 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
> * we have to overwrite values in the kernel image,
> * which are "rom"
> */
> - stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
> - stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
> + if (kernel_size > INITRD_PARM_SIZE + 8) {
> + stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
> + stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
> + }
> }
> }
> /*
I just had to discover that this breaks the argv handling in
kvm-unit-tests. Cornelia, could you please unqueue this patch again?
I'll try to come up with a better solution...
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [qemu-s390x] [PATCH v2] hw/s390x/ipl: Fix crashes that occurs when -kernel is used with small images
2018-06-14 19:08 ` [Qemu-devel] [qemu-s390x] " Thomas Huth
@ 2018-06-15 7:13 ` Cornelia Huck
2018-06-15 7:26 ` Thomas Huth
0 siblings, 1 reply; 6+ messages in thread
From: Cornelia Huck @ 2018-06-15 7:13 UTC (permalink / raw)
To: Thomas Huth; +Cc: Christian Borntraeger, qemu-s390x, qemu-devel
On Thu, 14 Jun 2018 21:08:43 +0200
Thomas Huth <thuth@redhat.com> wrote:
> On 11.06.2018 19:26, Thomas Huth wrote:
> > Add a sanity checks to fix the following two crashes:
> >
> > $ echo "Insane in the mainframe" > /tmp/test.txt
> > $ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt -append xyz
> > Segmentation fault (core dumped)
> > $ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt -initrd /tmp/test.txt
> > Segmentation fault (core dumped)
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> > v2: Fixed 2nd crash with -initrd
> >
> > hw/s390x/ipl.c | 9 ++++++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> > index 04245b5..2ff8f20 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);
> > @@ -195,8 +196,10 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
> > * we have to overwrite values in the kernel image,
> > * which are "rom"
> > */
> > - stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
> > - stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
> > + if (kernel_size > INITRD_PARM_SIZE + 8) {
> > + stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
> > + stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
> > + }
> > }
> > }
> > /*
>
> I just had to discover that this breaks the argv handling in
> kvm-unit-tests. Cornelia, could you please unqueue this patch again?
> I'll try to come up with a better solution...
Argh. I wonder why?
Unqueued.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [qemu-s390x] [PATCH v2] hw/s390x/ipl: Fix crashes that occurs when -kernel is used with small images
2018-06-15 7:13 ` Cornelia Huck
@ 2018-06-15 7:26 ` Thomas Huth
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2018-06-15 7:26 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Christian Borntraeger, qemu-s390x, qemu-devel
On 15.06.2018 09:13, Cornelia Huck wrote:
> On Thu, 14 Jun 2018 21:08:43 +0200
> Thomas Huth <thuth@redhat.com> wrote:
>
>> On 11.06.2018 19:26, Thomas Huth wrote:
>>> Add a sanity checks to fix the following two crashes:
>>>
>>> $ echo "Insane in the mainframe" > /tmp/test.txt
>>> $ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt -append xyz
>>> Segmentation fault (core dumped)
>>> $ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt -initrd /tmp/test.txt
>>> Segmentation fault (core dumped)
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> v2: Fixed 2nd crash with -initrd
>>>
>>> hw/s390x/ipl.c | 9 ++++++---
>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>> index 04245b5..2ff8f20 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);
>>> @@ -195,8 +196,10 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>> * we have to overwrite values in the kernel image,
>>> * which are "rom"
>>> */
>>> - stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
>>> - stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
>>> + if (kernel_size > INITRD_PARM_SIZE + 8) {
>>> + stq_p(rom_ptr(INITRD_PARM_START), initrd_offset);
>>> + stq_p(rom_ptr(INITRD_PARM_SIZE), initrd_size);
>>> + }
>>> }
>>> }
>>> /*
>>
>> I just had to discover that this breaks the argv handling in
>> kvm-unit-tests. Cornelia, could you please unqueue this patch again?
>> I'll try to come up with a better solution...
>
> Argh. I wonder why?
kvm-unit-tests are ELF files which are not loaded to address 0, but
still use the same entry point as Linux (0x10000) to be able to use the
"--append" option of QEMU. Thus the check "kernel_size > KERN_PARM_AREA
+ strlen(ipl->cmdline)" does not work here. We'd need something like
"start_addr + kernel_size > KERN_PARM_AREA + strlen(ipl->cmdline)"
instead - but this does not make too much sense with ELF files that have
multiple segments either.
The correct fix is to check the size when calling rom_ptr() instead. I
just sent a patch that does this.
Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-15 7:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-11 17:26 [Qemu-devel] [PATCH v2] hw/s390x/ipl: Fix crashes that occurs when -kernel is used with small images Thomas Huth
2018-06-12 7:37 ` [Qemu-devel] [qemu-s390x] " Christian Borntraeger
2018-06-12 9:26 ` [Qemu-devel] " Cornelia Huck
2018-06-14 19:08 ` [Qemu-devel] [qemu-s390x] " Thomas Huth
2018-06-15 7:13 ` Cornelia Huck
2018-06-15 7:26 ` Thomas Huth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).