Properly account for the possibility of multiboot kernels with a zero
bss_end_addr. The Multiboot Specification, section 3.1.3 allows for
kernels without a bss section, by allowing a zeroed bss_end_addr multiboot
header field.
Do some cleanup to multiboot.c as well:
- Remove some unused variables.
- Use more intuitive header names when displaying fields in messages.
- Change fprintf(stderr...) to error_report
Testing:
1) Ran the "make check" test suite.
2) Booted multiboot kernel with bss_end_addr=0. (I rolled my own
grub multiboot.elf test "kernel" by modifying source.) Verified
with gdb that new code that reads addresses/offsets from multiboot
header was accessed.
3) Booted multiboot kernel with non-zero bss_end_addr.
4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked.
5) Code has soaked in an internal repo for two months.
Thanks,
Jack
Jack Schwartz (4):
multiboot: bss_end_addr can be zero
multiboot: Remove unused variables from multiboot.c
multiboot: Use header names when displaying fields
multiboot: fprintf(stderr...) -> error_report()
hw/i386/multiboot.c | 77 ++++++++++++++++++++++++++---------------------------
1 file changed, 38 insertions(+), 39 deletions(-)
--
1.8.3.1
Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben: > Properly account for the possibility of multiboot kernels with a zero > bss_end_addr. The Multiboot Specification, section 3.1.3 allows for > kernels without a bss section, by allowing a zeroed bss_end_addr multiboot > header field. > > Do some cleanup to multiboot.c as well: > - Remove some unused variables. > - Use more intuitive header names when displaying fields in messages. > - Change fprintf(stderr...) to error_report > > Testing: > 1) Ran the "make check" test suite. > 2) Booted multiboot kernel with bss_end_addr=0. (I rolled my own > grub multiboot.elf test "kernel" by modifying source.) Verified > with gdb that new code that reads addresses/offsets from multiboot > header was accessed. > 3) Booted multiboot kernel with non-zero bss_end_addr. > 4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked. > 5) Code has soaked in an internal repo for two months. > > Thanks, > Jack Thanks, applied to my multiboot branch. Kevin
Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben: > Properly account for the possibility of multiboot kernels with a zero > bss_end_addr. The Multiboot Specification, section 3.1.3 allows for > kernels without a bss section, by allowing a zeroed bss_end_addr multiboot > header field. > > Do some cleanup to multiboot.c as well: > - Remove some unused variables. > - Use more intuitive header names when displaying fields in messages. > - Change fprintf(stderr...) to error_report There are some conflicts with Anatol's (CCed) multiboot series: https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html None if these should be hard to resolve, but it would be good if you could agree with each other whose patch series should come first, and then the other one should be rebased on top of that. > Testing: > 1) Ran the "make check" test suite. > 2) Booted multiboot kernel with bss_end_addr=0. (I rolled my own > grub multiboot.elf test "kernel" by modifying source.) Verified > with gdb that new code that reads addresses/offsets from multiboot > header was accessed. > 3) Booted multiboot kernel with non-zero bss_end_addr. > 4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked. > 5) Code has soaked in an internal repo for two months. Can you integrate your test kernel from 2) in tests/multiboot/ so we can keep this as a regression test? > Jack Schwartz (4): > multiboot: bss_end_addr can be zero > multiboot: Remove unused variables from multiboot.c > multiboot: Use header names when displaying fields > multiboot: fprintf(stderr...) -> error_report() Apart from the conflicts, the patches look good to me. Kevin
Hi Kevin and Anatol. Kevin, thanks for your review. More inline below... On 01/15/18 07:54, Kevin Wolf wrote: > Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben: >> Properly account for the possibility of multiboot kernels with a zero >> bss_end_addr. The Multiboot Specification, section 3.1.3 allows for >> kernels without a bss section, by allowing a zeroed bss_end_addr multiboot >> header field. >> >> Do some cleanup to multiboot.c as well: >> - Remove some unused variables. >> - Use more intuitive header names when displaying fields in messages. >> - Change fprintf(stderr...) to error_report > There are some conflicts with Anatol's (CCed) multiboot series: > https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html > > None if these should be hard to resolve, but it would be good if you > could agree with each other whose patch series should come first, and > then the other one should be rebased on top of that. Anatol, from my side, there are pros and cons to either patch set going in first, but advantages to either are pretty negligible. Pro for you going first: I can use the constants you will define in header files. Pro for me going first: your merge should be about the same as if you went first (since my changes are small, more localized and affect only multiboot.c) and my merge will be easier. What are your thoughts? > Testing: > 1) Ran the "make check" test suite. > 2) Booted multiboot kernel with bss_end_addr=0. (I rolled my own > grub multiboot.elf test "kernel" by modifying source.) Verified > with gdb that new code that reads addresses/offsets from multiboot > header was accessed. > 3) Booted multiboot kernel with non-zero bss_end_addr. > 4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked. > 5) Code has soaked in an internal repo for two months. > Can you integrate your test kernel from 2) in tests/multiboot/ so we can > keep this as a regression test? Kevin and alias, Before I proceed with adding my multiboot test file, I'll clarify here that I started with a version from the grub2 tree. In that file I expanded a header file, also from the same tree. Neither file had any license header, though the tree I got them from (Dated October 2017) contains the GNU GPLv3 license file. I'll need to check with my company before I can say I can deliver this file. If I deliver it, I'll add a header stating the GPL license, that it came from grub2 and to check its repository for contributors. >> Jack Schwartz (4): >> multiboot: bss_end_addr can be zero >> multiboot: Remove unused variables from multiboot.c >> multiboot: Use header names when displaying fields >> multiboot: fprintf(stderr...) -> error_report() > Apart from the conflicts, the patches look good to me. Thanks, Jack > > Kevin >
Am 17.01.2018 um 21:06 hat Jack Schwartz geschrieben: > Before I proceed with adding my multiboot test file, I'll clarify here that > I started with a version from the grub2 tree. In that file I expanded a > header file, also from the same tree. Neither file had any license header, > though the tree I got them from (Dated October 2017) contains the GNU GPLv3 > license file. I see. QEMU as a whole is GPLv2, so this might be a problem. It's probably not as bad as merging GPLv3 code into QEMU proper because it's a standalone test kernel that I suppose could have a different license. But IANAL and maybe it's safer not to go there. Maybe it would be less hassle to just reimplement the tests, based on the MIT licensed tests that are already in tests/multiboot/. Kevin
On Thu, Jan 18, 2018 at 12:35:00PM +0100, Kevin Wolf wrote: > Am 17.01.2018 um 21:06 hat Jack Schwartz geschrieben: > > Before I proceed with adding my multiboot test file, I'll clarify here that > > I started with a version from the grub2 tree. In that file I expanded a > > header file, also from the same tree. Neither file had any license header, > > though the tree I got them from (Dated October 2017) contains the GNU GPLv3 > > license file. > > I see. QEMU as a whole is GPLv2, so this might be a problem. It's > probably not as bad as merging GPLv3 code into QEMU proper because it's > a standalone test kernel that I suppose could have a different license. > But IANAL and maybe it's safer not to go there. As long as the GPLv3 code is not linked / otherwise combined with any of the GPLv2 code in QEMU that will be ok. Since it is a self-contained test kernel, that would not be an issue in this case - it only interfaces to QEMU via the x86 machine ABI. It just means that the QEMU source tar.gz will be under a conjunction of licenses "GPLv2 and GPLv2+ and GPLv3 and ...all our other licenss..." The resulting binaries for emulators/tools will still be GPLv2 as they're only linking in the GPLv2 + GPLv2+ source, never linking the GPlv3 test image. For simplicity of understanding though, it could be desirable to avoid this if not an unreasonable amount of extra work > Maybe it would be less hassle to just reimplement the tests, based on > the MIT licensed tests that are already in tests/multiboot/. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Hello Jack On Wed, Jan 17, 2018 at 12:06 PM, Jack Schwartz <jack.schwartz@oracle.com> wrote: > Hi Kevin and Anatol. > > Kevin, thanks for your review. > > More inline below... > > On 01/15/18 07:54, Kevin Wolf wrote: >> >> Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben: >>> >>> Properly account for the possibility of multiboot kernels with a zero >>> bss_end_addr. The Multiboot Specification, section 3.1.3 allows for >>> kernels without a bss section, by allowing a zeroed bss_end_addr >>> multiboot >>> header field. >>> >>> Do some cleanup to multiboot.c as well: >>> - Remove some unused variables. >>> - Use more intuitive header names when displaying fields in messages. >>> - Change fprintf(stderr...) to error_report >> >> There are some conflicts with Anatol's (CCed) multiboot series: >> https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html >> >> None if these should be hard to resolve, but it would be good if you >> could agree with each other whose patch series should come first, and >> then the other one should be rebased on top of that. > > Anatol, > > from my side, there are pros and cons to either patch set going in first, > but advantages to either are pretty negligible. Pro for you going first: I > can use the constants you will define in header files. Pro for me going > first: your merge should be about the same as if you went first (since my > changes are small, more localized and affect only multiboot.c) and my merge > will be easier. > > What are your thoughts? Please move ahead with your patches. I'll rebase my changes on top of yours. >> >> Testing: >> 1) Ran the "make check" test suite. >> 2) Booted multiboot kernel with bss_end_addr=0. (I rolled my own >> grub multiboot.elf test "kernel" by modifying source.) Verified >> with gdb that new code that reads addresses/offsets from multiboot >> header was accessed. >> 3) Booted multiboot kernel with non-zero bss_end_addr. >> 4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages >> worked. >> 5) Code has soaked in an internal repo for two months. >> Can you integrate your test kernel from 2) in tests/multiboot/ so we can >> keep this as a regression test? > > Kevin and alias, > > Before I proceed with adding my multiboot test file, I'll clarify here that > I started with a version from the grub2 tree. In that file I expanded a > header file, also from the same tree. Neither file had any license header, > though the tree I got them from (Dated October 2017) contains the GNU GPLv3 > license file. > > I'll need to check with my company before I can say I can deliver this file. > If I deliver it, I'll add a header stating the GPL license, that it came > from grub2 and to check its repository for contributors. >>> >>> Jack Schwartz (4): >>> multiboot: bss_end_addr can be zero >>> multiboot: Remove unused variables from multiboot.c >>> multiboot: Use header names when displaying fields >>> multiboot: fprintf(stderr...) -> error_report() >> >> Apart from the conflicts, the patches look good to me. > > Thanks, > Jack >> >> >> Kevin >> >
Hi Anatol, Daniel and Kevin. On 01/19/18 10:36, Anatol Pomozov wrote: > Hello Jack > > On Wed, Jan 17, 2018 at 12:06 PM, Jack Schwartz > <jack.schwartz@oracle.com> wrote: >> Hi Kevin and Anatol. >> >> Kevin, thanks for your review. >> >> More inline below... >> >> On 01/15/18 07:54, Kevin Wolf wrote: >>> Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben: >>>> Properly account for the possibility of multiboot kernels with a zero >>>> bss_end_addr. The Multiboot Specification, section 3.1.3 allows for >>>> kernels without a bss section, by allowing a zeroed bss_end_addr >>>> multiboot >>>> header field. >>>> >>>> Do some cleanup to multiboot.c as well: >>>> - Remove some unused variables. >>>> - Use more intuitive header names when displaying fields in messages. >>>> - Change fprintf(stderr...) to error_report >>> There are some conflicts with Anatol's (CCed) multiboot series: >>> https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html >>> >>> None if these should be hard to resolve, but it would be good if you >>> could agree with each other whose patch series should come first, and >>> then the other one should be rebased on top of that. >> Anatol, >> >> from my side, there are pros and cons to either patch set going in first, >> but advantages to either are pretty negligible. Pro for you going first: I >> can use the constants you will define in header files. Pro for me going >> first: your merge should be about the same as if you went first (since my >> changes are small, more localized and affect only multiboot.c) and my merge >> will be easier. >> >> What are your thoughts? > Please move ahead with your patches. I'll rebase my changes on top of yours. OK. I'm consulting with my company's legal department and waiting for their approvals for delivery of a test "kernel". I'll get in touch will everyone once I have an answer about that. I anticipate about a week before taking next steps to deliver. Kevin and Daniel, thanks for your inputs on this issue (different subthread), which I have forwarded to our legal department for review. Thanks, Jack > >>> Testing: >>> 1) Ran the "make check" test suite. >>> 2) Booted multiboot kernel with bss_end_addr=0. (I rolled my own >>> grub multiboot.elf test "kernel" by modifying source.) Verified >>> with gdb that new code that reads addresses/offsets from multiboot >>> header was accessed. >>> 3) Booted multiboot kernel with non-zero bss_end_addr. >>> 4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages >>> worked. >>> 5) Code has soaked in an internal repo for two months. >>> Can you integrate your test kernel from 2) in tests/multiboot/ so we can >>> keep this as a regression test? >> Kevin and alias, >> >> Before I proceed with adding my multiboot test file, I'll clarify here that >> I started with a version from the grub2 tree. In that file I expanded a >> header file, also from the same tree. Neither file had any license header, >> though the tree I got them from (Dated October 2017) contains the GNU GPLv3 >> license file. >> >> I'll need to check with my company before I can say I can deliver this file. >> If I deliver it, I'll add a header stating the GPL license, that it came >> from grub2 and to check its repository for contributors. >>>> Jack Schwartz (4): >>>> multiboot: bss_end_addr can be zero >>>> multiboot: Remove unused variables from multiboot.c >>>> multiboot: Use header names when displaying fields >>>> multiboot: fprintf(stderr...) -> error_report() >>> Apart from the conflicts, the patches look good to me. >> Thanks, >> Jack >>> >>> Kevin >>>
On Fri, Jan 19, 2018 at 04:18:07PM -0800, Jack Schwartz wrote: > Hi Anatol, Daniel and Kevin. > > On 01/19/18 10:36, Anatol Pomozov wrote: > > Hello Jack > > > > On Wed, Jan 17, 2018 at 12:06 PM, Jack Schwartz > > <jack.schwartz@oracle.com> wrote: > > > Hi Kevin and Anatol. > > > > > > Kevin, thanks for your review. > > > > > > More inline below... > > > > > > On 01/15/18 07:54, Kevin Wolf wrote: > > > > Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben: > > > > > Properly account for the possibility of multiboot kernels with a zero > > > > > bss_end_addr. The Multiboot Specification, section 3.1.3 allows for > > > > > kernels without a bss section, by allowing a zeroed bss_end_addr > > > > > multiboot > > > > > header field. > > > > > > > > > > Do some cleanup to multiboot.c as well: > > > > > - Remove some unused variables. > > > > > - Use more intuitive header names when displaying fields in messages. > > > > > - Change fprintf(stderr...) to error_report > > > > There are some conflicts with Anatol's (CCed) multiboot series: > > > > https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html > > > > > > > > None if these should be hard to resolve, but it would be good if you > > > > could agree with each other whose patch series should come first, and > > > > then the other one should be rebased on top of that. > > > Anatol, > > > > > > from my side, there are pros and cons to either patch set going in first, > > > but advantages to either are pretty negligible. Pro for you going first: I > > > can use the constants you will define in header files. Pro for me going > > > first: your merge should be about the same as if you went first (since my > > > changes are small, more localized and affect only multiboot.c) and my merge > > > will be easier. > > > > > > What are your thoughts? > > Please move ahead with your patches. I'll rebase my changes on top of yours. > OK. I'm consulting with my company's legal department and waiting for their > approvals for delivery of a test "kernel". I'll get in touch will everyone > once I have an answer about that. I anticipate about a week before taking > next steps to deliver. > > Kevin and Daniel, thanks for your inputs on this issue (different > subthread), which I have forwarded to our legal department for review. FWIW, I don't think it needs to be your responsibility to decide this. I think the QEMU community / maintainer taking the patches needs to decide whether it is acceptable / desirable. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Hi Kevin. On 2018-01-15 07:54, Kevin Wolf wrote: > Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben: >> Properly account for the possibility of multiboot kernels with a zero >> bss_end_addr. The Multiboot Specification, section 3.1.3 allows for >> kernels without a bss section, by allowing a zeroed bss_end_addr multiboot >> header field. >> >> Do some cleanup to multiboot.c as well: >> - Remove some unused variables. >> - Use more intuitive header names when displaying fields in messages. >> - Change fprintf(stderr...) to error_report > There are some conflicts with Anatol's (CCed) multiboot series: > https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html > > None if these should be hard to resolve, but it would be good if you > could agree with each other whose patch series should come first, and > then the other one should be rebased on top of that. > >> Testing: >> 1) Ran the "make check" test suite. >> 2) Booted multiboot kernel with bss_end_addr=0. (I rolled my own >> grub multiboot.elf test "kernel" by modifying source.) Verified >> with gdb that new code that reads addresses/offsets from multiboot >> header was accessed. >> 3) Booted multiboot kernel with non-zero bss_end_addr. >> 4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked. >> 5) Code has soaked in an internal repo for two months. > Can you integrate your test kernel from 2) in tests/multiboot/ so we can > keep this as a regression test? If need be, would you be willing to accept updated versions of these patches (with another review, of course) without the test file? I will deliver the test file later once I get company approvals. I don't want the test file to continue holding everything up in the meantime. Patchwork links, for reference: 1/4 multiboot: bss_end_addr can be zero http://patchwork.ozlabs.org/patch/852049/ 2/4 multiboot: Remove unused variables from multiboot.c http://patchwork.ozlabs.org/patch/852045/ 3/4 multiboot: Use header names when displaying fields http://patchwork.ozlabs.org/patch/852046/ 4/4 multiboot: fprintf(stderr...) -> error_report() http://patchwork.ozlabs.org/patch/852051/ Thanks, Jack > >> Jack Schwartz (4): >> multiboot: bss_end_addr can be zero >> multiboot: Remove unused variables from multiboot.c >> multiboot: Use header names when displaying fields >> multiboot: fprintf(stderr...) -> error_report() > Apart from the conflicts, the patches look good to me. > > Kevin >
Am 02.03.2018 um 20:32 hat Jack Schwartz geschrieben: > Hi Kevin. > > On 2018-01-15 07:54, Kevin Wolf wrote: > > Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben: > > > Properly account for the possibility of multiboot kernels with a zero > > > bss_end_addr. The Multiboot Specification, section 3.1.3 allows for > > > kernels without a bss section, by allowing a zeroed bss_end_addr multiboot > > > header field. > > > > > > Do some cleanup to multiboot.c as well: > > > - Remove some unused variables. > > > - Use more intuitive header names when displaying fields in messages. > > > - Change fprintf(stderr...) to error_report > > There are some conflicts with Anatol's (CCed) multiboot series: > > https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html > > > > None if these should be hard to resolve, but it would be good if you > > could agree with each other whose patch series should come first, and > > then the other one should be rebased on top of that. > > > > > Testing: > > > 1) Ran the "make check" test suite. > > > 2) Booted multiboot kernel with bss_end_addr=0. (I rolled my own > > > grub multiboot.elf test "kernel" by modifying source.) Verified > > > with gdb that new code that reads addresses/offsets from multiboot > > > header was accessed. > > > 3) Booted multiboot kernel with non-zero bss_end_addr. > > > 4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked. > > > 5) Code has soaked in an internal repo for two months. > > Can you integrate your test kernel from 2) in tests/multiboot/ so we can > > keep this as a regression test? > If need be, would you be willing to accept updated versions of these patches > (with another review, of course) without the test file? I will deliver the > test file later once I get company approvals. I don't want the test file to > continue holding everything up in the meantime. Sure, let's move forward with what we have now. Please keep me CCed when you send a new version and I'll give it a review and hopeuflly get it merged. Kevin
Hi Kevin and everyone. On 2018-03-05 00:13, Kevin Wolf wrote: > Am 02.03.2018 um 20:32 hat Jack Schwartz geschrieben: >> Hi Kevin. >> >> On 2018-01-15 07:54, Kevin Wolf wrote: >>> Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben: >>>> Properly account for the possibility of multiboot kernels with a zero >>>> bss_end_addr. The Multiboot Specification, section 3.1.3 allows for >>>> kernels without a bss section, by allowing a zeroed bss_end_addr multiboot >>>> header field. >>>> >>>> Do some cleanup to multiboot.c as well: >>>> - Remove some unused variables. >>>> - Use more intuitive header names when displaying fields in messages. >>>> - Change fprintf(stderr...) to error_report >>> There are some conflicts with Anatol's (CCed) multiboot series: >>> https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg03003.html >>> >>> None if these should be hard to resolve, but it would be good if you >>> could agree with each other whose patch series should come first, and >>> then the other one should be rebased on top of that. >>> >>>> Testing: >>>> 1) Ran the "make check" test suite. >>>> 2) Booted multiboot kernel with bss_end_addr=0. (I rolled my own >>>> grub multiboot.elf test "kernel" by modifying source.) Verified >>>> with gdb that new code that reads addresses/offsets from multiboot >>>> header was accessed. >>>> 3) Booted multiboot kernel with non-zero bss_end_addr. >>>> 4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked. >>>> 5) Code has soaked in an internal repo for two months. >>> Can you integrate your test kernel from 2) in tests/multiboot/ so we can >>> keep this as a regression test? >> If need be, would you be willing to accept updated versions of these patches >> (with another review, of course) without the test file? I will deliver the >> test file later once I get company approvals. I don't want the test file to >> continue holding everything up in the meantime. > Sure, let's move forward with what we have now. Please keep me CCed when > you send a new version and I'll give it a review and hopeuflly get it > merged. > > Kevin Thanks, Kevin. Patches have not changed, and I verified they still work on a current repo. (Multiboot.c has had a one-line change regarding a header file, so I rebuilt and re-tested to make sure.) Links again, for your reference: 1/4 multiboot: bss_end_addr can be zero http://patchwork.ozlabs.org/patch/852049/ 2/4 multiboot: Remove unused variables from multiboot.c http://patchwork.ozlabs.org/patch/852045/ 3/4 multiboot: Use header names when displaying fields http://patchwork.ozlabs.org/patch/852046/ 4/4 multiboot: fprintf(stderr...) -> error_report() http://patchwork.ozlabs.org/patch/852051/ Thanks, Jack
Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben: > Properly account for the possibility of multiboot kernels with a zero > bss_end_addr. The Multiboot Specification, section 3.1.3 allows for > kernels without a bss section, by allowing a zeroed bss_end_addr multiboot > header field. > > Do some cleanup to multiboot.c as well: > - Remove some unused variables. > - Use more intuitive header names when displaying fields in messages. > - Change fprintf(stderr...) to error_report [ Cc: qemu-stable ] This series happens to fix CVE-2018-7550. http://www.openwall.com/lists/oss-security/2018/03/08/4 Just a shame that we weren't told before merging it so that the appropriate tags could have been set in the commit message (and all of the problems could have been addressed; I'm going to send another Multiboot series now). Kevin
On March 14, 2018 1:23:51 PM EDT, Kevin Wolf <kwolf@redhat.com> wrote: >Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben: >> Properly account for the possibility of multiboot kernels with a zero >> bss_end_addr. The Multiboot Specification, section 3.1.3 allows for >> kernels without a bss section, by allowing a zeroed bss_end_addr >multiboot >> header field. >> >> Do some cleanup to multiboot.c as well: >> - Remove some unused variables. >> - Use more intuitive header names when displaying fields in messages. >> - Change fprintf(stderr...) to error_report > >[ Cc: qemu-stable ] > >This series happens to fix CVE-2018-7550. >http://www.openwall.com/lists/oss-security/2018/03/08/4 > >Just a shame that we weren't told before merging it so that the >appropriate tags could have been set in the commit message (and all of >the problems could have been addressed; I'm going to send another >Multiboot series now). Huh? You mean the CVE tags that were created in 2018 for a patch posted in 2017? Or that the reporter of the security issue didn't point to this particular patch? Irrespective of that, is there a write-up of how security process works at QEMU? That is what is the usual embargo period, the list of security folks, how one can become one, what are the responsibilities, how changes to process are being carried out (and discussed), what breath of testing and PoC work is done , how security fixes are being reviewed, etc? Thanks! > >Kevin
Am 14.03.2018 um 18:35 hat Konrad Rzeszutek Wilk geschrieben: > On March 14, 2018 1:23:51 PM EDT, Kevin Wolf <kwolf@redhat.com> wrote: > >Am 21.12.2017 um 18:25 hat Jack Schwartz geschrieben: > >> Properly account for the possibility of multiboot kernels with a zero > >> bss_end_addr. The Multiboot Specification, section 3.1.3 allows for > >> kernels without a bss section, by allowing a zeroed bss_end_addr > >multiboot > >> header field. > >> > >> Do some cleanup to multiboot.c as well: > >> - Remove some unused variables. > >> - Use more intuitive header names when displaying fields in messages. > >> - Change fprintf(stderr...) to error_report > > > >[ Cc: qemu-stable ] > > > >This series happens to fix CVE-2018-7550. > >http://www.openwall.com/lists/oss-security/2018/03/08/4 > > > >Just a shame that we weren't told before merging it so that the > >appropriate tags could have been set in the commit message (and all of > >the problems could have been addressed; I'm going to send another > >Multiboot series now). > > Huh? > > You mean the CVE tags that were created in 2018 for a patch posted in > 2017? Well, it seems to me that this patch was created for a different purpose, but it happens to fix the bug for which this CVE was assigned now. It's not your or Jack's fault, that's just how things go sometimes. I think PJP knew that this CVE was coming before the patches were merged into master, so if he had told us, we could have had a better commit message. But either way, it's not a disaster to have a suboptimal commit message. > Or that the reporter of the security issue didn't point to this particular patch? > > Irrespective of that, is there a write-up of how security process > works at QEMU? > > That is what is the usual embargo period, the list of security folks, > how one can become one, what are the responsibilities, how changes to > process are being carried out (and discussed), what breath of testing > and PoC work is done , how security fixes are being reviewed, etc? I don't think a problem like this would be embargoed at all. Anyway, have a look here: https://wiki.qemu.org/SecurityProcess Kevin
+-- On Wed, 14 Mar 2018, Kevin Wolf wrote --+ | Well, it seems to me that this patch was created for a different | purpose, but it happens to fix the bug for which this CVE was assigned | now. Right. I had sent another patch to fix it, there Jack mentioned about his series from before. | I think PJP knew that this CVE was coming before the patches were merged | into master, so if he had told us, we could have had a better commit | message. For non-embargoed issues we request CVE from Mitre after a proposed patch is reviewed and acked for upstream QEMU. Nevertheless, going forward I'll mention the CVE-ID here on the list, as soon as it is assigned. | I don't think a problem like this would be embargoed at all. True. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Hi everyone. Ping! Please review. Patchwork links: 1/4 multiboot: bss_end_addr can be zero http://patchwork.ozlabs.org/patch/852049/ 2/4 multiboot: Remove unused variables from multiboot.c http://patchwork.ozlabs.org/patch/852045/ 3/4 multiboot: Use header names when displaying fields http://patchwork.ozlabs.org/patch/852046/ 4/4 multiboot: fprintf(stderr...) -> error_report() http://patchwork.ozlabs.org/patch/852051/ Thanks, Jack On 12/21/17 09:25, Jack Schwartz wrote: > Properly account for the possibility of multiboot kernels with a zero > bss_end_addr. The Multiboot Specification, section 3.1.3 allows for > kernels without a bss section, by allowing a zeroed bss_end_addr multiboot > header field. > > Do some cleanup to multiboot.c as well: > - Remove some unused variables. > - Use more intuitive header names when displaying fields in messages. > - Change fprintf(stderr...) to error_report > > Testing: > 1) Ran the "make check" test suite. > 2) Booted multiboot kernel with bss_end_addr=0. (I rolled my own > grub multiboot.elf test "kernel" by modifying source.) Verified > with gdb that new code that reads addresses/offsets from multiboot > header was accessed. > 3) Booted multiboot kernel with non-zero bss_end_addr. > 4) Uncommented DEBUG_MULTIBOOT in multiboot.c and verified messages worked. > 5) Code has soaked in an internal repo for two months. > > Thanks, > Jack > > Jack Schwartz (4): > multiboot: bss_end_addr can be zero > multiboot: Remove unused variables from multiboot.c > multiboot: Use header names when displaying fields > multiboot: fprintf(stderr...) -> error_report() > > hw/i386/multiboot.c | 77 ++++++++++++++++++++++++++--------------------------- > 1 file changed, 38 insertions(+), 39 deletions(-) >
© 2016 - 2024 Red Hat, Inc.