| Message ID | 88d9afed-f91d-c320-13c8-9a93fc52b700@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/25/2018 09:27 AM, Christian Borntraeger wrote: > Also adding QEMU. > > On 06/25/2018 09:10 AM, Christian Borntraeger wrote: >> >> >> On 06/22/2018 09:47 PM, Guenter Roeck wrote: >>> Hi, >>> >>> starting with commit 's390/boot: make head.S and als.c be part of the >>> decompressor only' in -next, s390 immages no longer boot in qemu. >>> As far as I can see, the reason is that the command line is no longer >>> passed from qemu to the kernel, which results in a panic because the >>> root file system can not be mounted. >>> >>> Was this change made on purpose ? If so, is there a way to get qemu >>> back to working ? >> >> Certainly not on purpose. >> >> Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file) >> >> e.g. >> >> qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line" >> >> The compressed image (bzImage) seems to work fine though. >> >> This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start >> address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file >> the load address is 0x100000 as there is no unpacker. >> >> Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround? > > Something like this in QEMU > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index f278036fa7..14153ce880 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) > */ > if (pentry == KERN_IMAGE_START || pentry == 0x800) { > ipl->start_addr = KERN_IMAGE_START; > - /* Overwrite parameters in the kernel image, which are "rom" */ > - strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > } else { > ipl->start_addr = pentry; > } > + if (ipl->cmdline) { > + /* If there is a command line, put it in the right place */ > + strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > + } > > if (ipl->initrd) { > ram_addr_t initrd_offset; > > would put the command line in no matter what the start address is. Ideally we would do 2 changes: - change QEMU to add a commandline to 10480 if specified - have a way to fix the kernel elf file to still boot with older QEMUs
On Mon, 25 Jun 2018 09:27:59 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > Also adding QEMU. > > On 06/25/2018 09:10 AM, Christian Borntraeger wrote: > > > > > > On 06/22/2018 09:47 PM, Guenter Roeck wrote: > >> Hi, > >> > >> starting with commit 's390/boot: make head.S and als.c be part of the > >> decompressor only' in -next, s390 immages no longer boot in qemu. > >> As far as I can see, the reason is that the command line is no longer > >> passed from qemu to the kernel, which results in a panic because the > >> root file system can not be mounted. > >> > >> Was this change made on purpose ? If so, is there a way to get qemu > >> back to working ? > > > > Certainly not on purpose. > > > > Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file) > > > > e.g. > > > > qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line" > > > > The compressed image (bzImage) seems to work fine though. > > > > This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start > > address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file > > the load address is 0x100000 as there is no unpacker. Do we consider these locations to be an exported interface, or is it just QEMU guessing? > > > > Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround? > > Something like this in QEMU > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index f278036fa7..14153ce880 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) > */ > if (pentry == KERN_IMAGE_START || pentry == 0x800) { > ipl->start_addr = KERN_IMAGE_START; > - /* Overwrite parameters in the kernel image, which are "rom" */ > - strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > } else { > ipl->start_addr = pentry; > } > + if (ipl->cmdline) { > + /* If there is a command line, put it in the right place */ > + strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > + } Check for the magic Linux string (like in the non-elf case) first? > > if (ipl->initrd) { > ram_addr_t initrd_offset; > > would put the command line in no matter what the start address is. I'm for putting that one in (and backporting it to qemu-stable). It's a bit worrying, though, that our ipl code is so fragile...
On Mon, 25 Jun 2018 10:02:28 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 06/25/2018 09:27 AM, Christian Borntraeger wrote: > > Also adding QEMU. > > > > On 06/25/2018 09:10 AM, Christian Borntraeger wrote: > >> > >> > >> On 06/22/2018 09:47 PM, Guenter Roeck wrote: > >>> Hi, > >>> > >>> starting with commit 's390/boot: make head.S and als.c be part of the > >>> decompressor only' in -next, s390 immages no longer boot in qemu. > >>> As far as I can see, the reason is that the command line is no longer > >>> passed from qemu to the kernel, which results in a panic because the > >>> root file system can not be mounted. > >>> > >>> Was this change made on purpose ? If so, is there a way to get qemu > >>> back to working ? > >> > >> Certainly not on purpose. > >> > >> Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file) > >> > >> e.g. > >> > >> qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line" > >> > >> The compressed image (bzImage) seems to work fine though. > >> > >> This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start > >> address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file > >> the load address is 0x100000 as there is no unpacker. > >> > >> Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround? > > > > Something like this in QEMU > > > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > > index f278036fa7..14153ce880 100644 > > --- a/hw/s390x/ipl.c > > +++ b/hw/s390x/ipl.c > > @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) > > */ > > if (pentry == KERN_IMAGE_START || pentry == 0x800) { > > ipl->start_addr = KERN_IMAGE_START; > > - /* Overwrite parameters in the kernel image, which are "rom" */ > > - strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > > } else { > > ipl->start_addr = pentry; > > } > > + if (ipl->cmdline) { > > + /* If there is a command line, put it in the right place */ > > + strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > > + } > > > > if (ipl->initrd) { > > ram_addr_t initrd_offset; > > > > would put the command line in no matter what the start address is. > > Ideally we would do 2 changes: > - change QEMU to add a commandline to 10480 if specified > - have a way to fix the kernel elf file to still boot with older QEMUs > Agreed on both. Even if we change QEMU now (+ stable), there are many versions out there that will now fail.
On 25.06.2018 10:02, Christian Borntraeger wrote: > > > On 06/25/2018 09:27 AM, Christian Borntraeger wrote: >> Also adding QEMU. >> >> On 06/25/2018 09:10 AM, Christian Borntraeger wrote: >>> >>> >>> On 06/22/2018 09:47 PM, Guenter Roeck wrote: >>>> Hi, >>>> >>>> starting with commit 's390/boot: make head.S and als.c be part of the >>>> decompressor only' in -next, s390 immages no longer boot in qemu. >>>> As far as I can see, the reason is that the command line is no longer >>>> passed from qemu to the kernel, which results in a panic because the >>>> root file system can not be mounted. >>>> >>>> Was this change made on purpose ? If so, is there a way to get qemu >>>> back to working ? >>> >>> Certainly not on purpose. >>> >>> Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file) >>> >>> e.g. >>> >>> qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line" >>> >>> The compressed image (bzImage) seems to work fine though. >>> >>> This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start >>> address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file >>> the load address is 0x100000 as there is no unpacker. >>> >>> Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround? >> >> Something like this in QEMU >> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >> index f278036fa7..14153ce880 100644 >> --- a/hw/s390x/ipl.c >> +++ b/hw/s390x/ipl.c >> @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) >> */ >> if (pentry == KERN_IMAGE_START || pentry == 0x800) { >> ipl->start_addr = KERN_IMAGE_START; >> - /* Overwrite parameters in the kernel image, which are "rom" */ >> - strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); >> } else { >> ipl->start_addr = pentry; >> } >> + if (ipl->cmdline) { >> + /* If there is a command line, put it in the right place */ >> + strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); You definitely need to check for rom_ptr() != NULL first before calling strcpy. Otherwise QEMU segfaults in case the user tried to load a kernel to a different location. See also: https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg04227.html Thomas
On 06/25/2018 10:05 AM, Cornelia Huck wrote: > On Mon, 25 Jun 2018 09:27:59 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> Also adding QEMU. >> >> On 06/25/2018 09:10 AM, Christian Borntraeger wrote: >>> >>> >>> On 06/22/2018 09:47 PM, Guenter Roeck wrote: >>>> Hi, >>>> >>>> starting with commit 's390/boot: make head.S and als.c be part of the >>>> decompressor only' in -next, s390 immages no longer boot in qemu. >>>> As far as I can see, the reason is that the command line is no longer >>>> passed from qemu to the kernel, which results in a panic because the >>>> root file system can not be mounted. >>>> >>>> Was this change made on purpose ? If so, is there a way to get qemu >>>> back to working ? >>> >>> Certainly not on purpose. >>> >>> Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file) >>> >>> e.g. >>> >>> qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line" >>> >>> The compressed image (bzImage) seems to work fine though. >>> >>> This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start >>> address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file >>> the load address is 0x100000 as there is no unpacker. > > Do we consider these locations to be an exported interface, or is it > just QEMU guessing? It is using what zipl has hardcoded. This works fine for the final image (with the unpacker), but being able to boot the elf file was just a very nice feature of QEMU. > >>> >>> Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround? >> >> Something like this in QEMU >> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >> index f278036fa7..14153ce880 100644 >> --- a/hw/s390x/ipl.c >> +++ b/hw/s390x/ipl.c >> @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) >> */ >> if (pentry == KERN_IMAGE_START || pentry == 0x800) { >> ipl->start_addr = KERN_IMAGE_START; >> - /* Overwrite parameters in the kernel image, which are "rom" */ >> - strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); >> } else { >> ipl->start_addr = pentry; >> } >> + if (ipl->cmdline) { >> + /* If there is a command line, put it in the right place */ >> + strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); >> + } > > Check for the magic Linux string (like in the non-elf case) first? Even that does not exists in vmlinux but only in bzImage with the latest patchset (in next, but not upstream yet) > >> >> if (ipl->initrd) { >> ram_addr_t initrd_offset; >> >> would put the command line in no matter what the start address is. > > I'm for putting that one in (and backporting it to qemu-stable). It's a > bit worrying, though, that our ipl code is so fragile... We actually have to combine this with Thomas fix (to check for rom_ptr returning something sane). It seems that ipl->commandline is always there, so we have to check for strlen!=0 it seems.. I mean if somebody ask for "-append something" we can certainly always write something if there is rom/ram.
This change has been done on purpose. Uncompressed image is not going to be bootable any more. In future the decompressor phase would get more function (early memory detection as an example) and there is no chance to duplicate that code in uncompressed image as well (to keep it bootable on its own). The patch series commit messages contain more technical details. For qemu either bzImage or arch/s390/boot/compressed/vmlinux should be used, which are bootable images. But that's really confusing that uncompressed vmlinux is still kind of booting. May be we should discuss how to avoid this confusion (may be change uncompressed image enty point to a function doing disabled wait with badb007 or smth) and how to encourage people to use arch/s390/boot/compressed/vmlinux instead. On Mon, Jun 25, 2018 at 10:05:48AM +0200, Cornelia Huck wrote: > On Mon, 25 Jun 2018 09:27:59 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > Also adding QEMU. > > > > On 06/25/2018 09:10 AM, Christian Borntraeger wrote: > > > > > > > > > On 06/22/2018 09:47 PM, Guenter Roeck wrote: > > >> Hi, > > >> > > >> starting with commit 's390/boot: make head.S and als.c be part of the > > >> decompressor only' in -next, s390 immages no longer boot in qemu. > > >> As far as I can see, the reason is that the command line is no longer > > >> passed from qemu to the kernel, which results in a panic because the > > >> root file system can not be mounted. > > >> > > >> Was this change made on purpose ? If so, is there a way to get qemu > > >> back to working ? > > > > > > Certainly not on purpose. > > > > > > Vasily, I can reproduce this with KVM and an external kernel boot of the vmlinux file (the elf file) > > > > > > e.g. > > > > > > qemu-system-s390 -enable-kvm -nographic -kernel vmlinux -append "this string no longer is command line" > > > > > > The compressed image (bzImage) seems to work fine though. > > > > > > This seems to be an unfortunate side effect of QEMUs ways to "guess" its Linux (checking for start > > > address 0x10000, which is no longer true for the vmlinux file). With the pure vmlinux elf file > > > the load address is 0x100000 as there is no unpacker. > > Do we consider these locations to be an exported interface, or is it > just QEMU guessing? > > > > > > > Guenter, can you check if arch/s390/boot/bzImage works for you as a workaround? > > > > Something like this in QEMU > > > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > > index f278036fa7..14153ce880 100644 > > --- a/hw/s390x/ipl.c > > +++ b/hw/s390x/ipl.c > > @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) > > */ > > if (pentry == KERN_IMAGE_START || pentry == 0x800) { > > ipl->start_addr = KERN_IMAGE_START; > > - /* Overwrite parameters in the kernel image, which are "rom" */ > > - strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > > } else { > > ipl->start_addr = pentry; > > } > > + if (ipl->cmdline) { > > + /* If there is a command line, put it in the right place */ > > + strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > > + } > > Check for the magic Linux string (like in the non-elf case) first? > > > > > if (ipl->initrd) { > > ram_addr_t initrd_offset; > > > > would put the command line in no matter what the start address is. > > I'm for putting that one in (and backporting it to qemu-stable). It's a > bit worrying, though, that our ipl code is so fragile... >
On Mon, 25 Jun 2018 10:36:33 +0200 Vasily Gorbik <gor@linux.ibm.com> wrote: > This change has been done on purpose. Uncompressed image is not going > to be bootable any more. In future the decompressor phase would get > more function (early memory detection as an example) and there is no > chance to duplicate that code in uncompressed image as well (to keep it > bootable on its own). The patch series commit messages contain more > technical details. > > For qemu either bzImage or arch/s390/boot/compressed/vmlinux should be > used, which are bootable images. > > But that's really confusing that uncompressed vmlinux is still kind > of booting. May be we should discuss how to avoid this confusion > (may be change uncompressed image enty point to a function doing > disabled wait with badb007 or smth) and how to encourage people to use > arch/s390/boot/compressed/vmlinux instead. So, the intention is that you can't boot the uncompressed image anywhere? (Was it possible before, e.g. when punching the image under z/VM?) If yes, it would make sense to explicitly fence it. But I'm worried that it would break previously working setups (did we document the purpose of the images anywhere?)
On 06/25/2018 10:49 AM, Cornelia Huck wrote: > On Mon, 25 Jun 2018 10:36:33 +0200 > Vasily Gorbik <gor@linux.ibm.com> wrote: > >> This change has been done on purpose. Uncompressed image is not going >> to be bootable any more. In future the decompressor phase would get >> more function (early memory detection as an example) and there is no >> chance to duplicate that code in uncompressed image as well (to keep it >> bootable on its own). The patch series commit messages contain more >> technical details. >> >> For qemu either bzImage or arch/s390/boot/compressed/vmlinux should be >> used, which are bootable images. >> >> But that's really confusing that uncompressed vmlinux is still kind >> of booting. May be we should discuss how to avoid this confusion >> (may be change uncompressed image enty point to a function doing >> disabled wait with badb007 or smth) and how to encourage people to use >> arch/s390/boot/compressed/vmlinux instead. > > So, the intention is that you can't boot the uncompressed image > anywhere? (Was it possible before, e.g. when punching the image under > z/VM?) The uncompressed image (the vmlinux file) was never bootable in LPAR or z/VM. It was just a "nice hack" that QEMU was able to do so. (even qemu on x86 can not boot the pure vmlinux file as far as I know). I talked to Vasily and the vmlinux file in the linux source path is just an intermediate file. Future changes will happen that will make that ELF file unsuitable for direct boot anyway (e.g. think about potential ASLR or Kasan changes). If yes, it would make sense to explicitly fence it. But I'm > worried that it would break previously working setups (did we document > the purpose of the images anywhere?) > I think by referring to arch/s390/boot/compressed/vmlinux things are probably good enough. That will still load from 0x10000. We might still "change" the way that we add the parameters (e.g. make that not depend on the load address), but looking forward this might become less important for the "intermediate vmlnux file".
On 06/25/2018 05:26 AM, Christian Borntraeger wrote: > > > On 06/25/2018 10:49 AM, Cornelia Huck wrote: >> On Mon, 25 Jun 2018 10:36:33 +0200 >> Vasily Gorbik <gor@linux.ibm.com> wrote: >> >>> This change has been done on purpose. Uncompressed image is not going >>> to be bootable any more. In future the decompressor phase would get >>> more function (early memory detection as an example) and there is no >>> chance to duplicate that code in uncompressed image as well (to keep it >>> bootable on its own). The patch series commit messages contain more >>> technical details. >>> >>> For qemu either bzImage or arch/s390/boot/compressed/vmlinux should be >>> used, which are bootable images. >>> >>> But that's really confusing that uncompressed vmlinux is still kind >>> of booting. May be we should discuss how to avoid this confusion >>> (may be change uncompressed image enty point to a function doing >>> disabled wait with badb007 or smth) and how to encourage people to use >>> arch/s390/boot/compressed/vmlinux instead. >> >> So, the intention is that you can't boot the uncompressed image >> anywhere? (Was it possible before, e.g. when punching the image under >> z/VM?) > > The uncompressed image (the vmlinux file) was never bootable in LPAR or z/VM. > It was just a "nice hack" that QEMU was able to do so. (even qemu on x86 can not > boot the pure vmlinux file as far as I know). > "even" is relative. vmlinux boots on some arm platforms, metag, mips64, nios2, parisc, ppc/ppc64, and riscv. If an image is not expected to be bootable, a message such as "This image does not boot. Please use <correct image>" would be nice. Unfortunately, which image to boot under qemu is pretty much undocumented, and it is guesswork for each architecture/platform. Guenter > I talked to Vasily and the vmlinux file in the linux source path is just an > intermediate file. Future changes will happen that will make that ELF file > unsuitable for direct boot anyway (e.g. think about potential ASLR or Kasan > changes). > > If yes, it would make sense to explicitly fence it. But I'm >> worried that it would break previously working setups (did we document >> the purpose of the images anywhere? >> > > > I think by referring to arch/s390/boot/compressed/vmlinux things are probably > good enough. That will still load from 0x10000. > > We might still "change" the way that we add the parameters (e.g. > make that not depend on the load address), but looking forward this might > become less important for the "intermediate vmlnux file". > >
On Mon, Jun 25, 2018 at 06:35:30AM -0700, Guenter Roeck wrote: > On 06/25/2018 05:26 AM, Christian Borntraeger wrote: > > > > > > On 06/25/2018 10:49 AM, Cornelia Huck wrote: > > > On Mon, 25 Jun 2018 10:36:33 +0200 > > > Vasily Gorbik <gor@linux.ibm.com> wrote: > > > > > > > This change has been done on purpose. Uncompressed image is not going > > > > to be bootable any more. In future the decompressor phase would get > > > > more function (early memory detection as an example) and there is no > > > > chance to duplicate that code in uncompressed image as well (to keep it > > > > bootable on its own). The patch series commit messages contain more > > > > technical details. > > > > > > > > For qemu either bzImage or arch/s390/boot/compressed/vmlinux should be > > > > used, which are bootable images. > > > > > > > > But that's really confusing that uncompressed vmlinux is still kind > > > > of booting. May be we should discuss how to avoid this confusion > > > > (may be change uncompressed image enty point to a function doing > > > > disabled wait with badb007 or smth) and how to encourage people to use > > > > arch/s390/boot/compressed/vmlinux instead. > > > > > If an image is not expected to be bootable, a message such as "This image does > not boot. Please use <correct image>" would be nice. Unfortunately, which image > to boot under qemu is pretty much undocumented, and it is guesswork for each > architecture/platform. > > Guenter > > > I talked to Vasily and the vmlinux file in the linux source path is just an > > intermediate file. Future changes will happen that will make that ELF file > > unsuitable for direct boot anyway (e.g. think about potential ASLR or Kasan > > changes). > > > > If yes, it would make sense to explicitly fence it. But I'm > > > worried that it would break previously working setups (did we document > > > the purpose of the images anywhere? > > > To avoid confusion with trying to boot uncompressed vmlinux under qemu we could detect it and print "nice" message in the kernel. Please consider the following patch. Vasily Gorbik (1): s390/boot: block uncompressed vmlinux booting attempts arch/s390/boot/head.S | 4 ++-- arch/s390/include/asm/setup.h | 3 ++- arch/s390/kernel/early.c | 12 ++++++++++++ 3 files changed, 16 insertions(+), 3 deletions(-)
On Mon, Jun 25, 2018 at 09:27:59AM +0200, Christian Borntraeger wrote: > - /* Overwrite parameters in the kernel image, which are "rom" */ > - strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > + strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); Why not replace strcpy() with strncpy() or snprintf()? strcpy() may overflow.
On 26.06.2018 07:32, Georgi Guninski wrote: > On Mon, Jun 25, 2018 at 09:27:59AM +0200, Christian Borntraeger wrote: >> - /* Overwrite parameters in the kernel image, which are "rom" */ >> - strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > >> + strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > > Why not replace strcpy() with strncpy() or snprintf()? > strcpy() may overflow. This will be fixed by https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg04227.html by adding a check for a valid size. Thomas
On Mon, 25 Jun 2018 10:29:46 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 06/25/2018 10:05 AM, Cornelia Huck wrote: > > On Mon, 25 Jun 2018 09:27:59 +0200 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> Something like this in QEMU > >> > >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > >> index f278036fa7..14153ce880 100644 > >> --- a/hw/s390x/ipl.c > >> +++ b/hw/s390x/ipl.c > >> @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) > >> */ > >> if (pentry == KERN_IMAGE_START || pentry == 0x800) { > >> ipl->start_addr = KERN_IMAGE_START; > >> - /* Overwrite parameters in the kernel image, which are "rom" */ > >> - strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > >> } else { > >> ipl->start_addr = pentry; > >> } > >> + if (ipl->cmdline) { > >> + /* If there is a command line, put it in the right place */ > >> + strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); > >> + } > > > > Check for the magic Linux string (like in the non-elf case) first? > > Even that does not exists in vmlinux but only in bzImage with the latest patchset > (in next, but not upstream yet) Ok. > > > >> > >> if (ipl->initrd) { > >> ram_addr_t initrd_offset; > >> > >> would put the command line in no matter what the start address is. > > > > I'm for putting that one in (and backporting it to qemu-stable). It's a > > bit worrying, though, that our ipl code is so fragile... > > We actually have to combine this with Thomas fix (to check for rom_ptr returning > something sane). It seems that ipl->commandline is always there, so we have to > check for strlen!=0 it seems.. > > I mean if somebody ask for "-append something" we can certainly always write something > if there is rom/ram. Given that the uncompressed image is not supposed to be bootable anymore, does it make sense to add this anyway? I'll go ahead and queue Thomas' fix, though.
On 06/26/2018 10:29 AM, Cornelia Huck wrote: [...] >>>> >>>> if (ipl->initrd) { >>>> ram_addr_t initrd_offset; >>>> >>>> would put the command line in no matter what the start address is. >>> >>> I'm for putting that one in (and backporting it to qemu-stable). It's a >>> bit worrying, though, that our ipl code is so fragile... >> >> We actually have to combine this with Thomas fix (to check for rom_ptr returning >> something sane). It seems that ipl->commandline is always there, so we have to >> check for strlen!=0 it seems.. >> >> I mean if somebody ask for "-append something" we can certainly always write something >> if there is rom/ram. > > Given that the uncompressed image is not supposed to be bootable > anymore, does it make sense to add this anyway? I think we can drop this patch for now. > > I'll go ahead and queue Thomas' fix, though. yes, please
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index f278036fa7..14153ce880 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -187,11 +187,13 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp) */ if (pentry == KERN_IMAGE_START || pentry == 0x800) { ipl->start_addr = KERN_IMAGE_START; - /* Overwrite parameters in the kernel image, which are "rom" */ - strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); } else { ipl->start_addr = pentry; } + if (ipl->cmdline) { + /* If there is a command line, put it in the right place */ + strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline); + } if (ipl->initrd) { ram_addr_t initrd_offset;