| Message ID | 20220728093558.1163751-1-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-7.1] applesmc: silence invalid key warning in case default one is used | expand |
On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote: > QEMU probably can't carry OSK key[1] for legal reasons so it > can't supply the valid default key. However when tests are run > applesmc will pollute test log with distracting warning, > silence that warning so it won't distract maintainers/CI. What test is causing this problem ? > > If user launches MacOS guest without supplying valid or any key > they will get upset MacOS (and legality of specifying valid key > on CLI is also questionable). This patch favors QEMU/tests vs > MacOS guest. > > A patch from Apple needed to change default to the valid key, > until then prefer QEMU maintainers sanity/cleaner test logs > vs any inconvenience users running MacOS guest might suffer. > > 1) After some googling, it seems 'secret' key is the same for > all Mac hardware and is widely known but whether it's > permited to use that is up to lawyers/Apple to decide. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/misc/applesmc.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c > index 5f9c742e50..bce27b0950 100644 > --- a/hw/misc/applesmc.c > +++ b/hw/misc/applesmc.c > @@ -84,7 +84,7 @@ enum { > #define smc_debug(...) do { } while (0) > #endif > > -static char default_osk[64] = "This is a dummy key. Enter the real key " > +static char default_osk[65] = "This is a dummy key. Enter the real key " > "using the -osk parameter"; This is a distinct bug fix from the problem described. We should take this as a standalone fix no matter what. > struct AppleSMCData { > @@ -335,10 +335,12 @@ static void applesmc_isa_realize(DeviceState *dev, Error **errp) > isa_register_ioport(&s->parent_obj, &s->io_err, > s->iobase + APPLESMC_ERR_PORT); > > - if (!s->osk || (strlen(s->osk) != 64)) { > - warn_report("Using AppleSMC with invalid key"); > + if (!s->osk) { > s->osk = default_osk; > } > + if (strlen(s->osk) != 64) { > + warn_report("Using AppleSMC with invalid key"); > + } This means users who invoke QEMU won't see the warning any more. This feels like a dubious change, as it degrades the user experiance for sake of tests - IMHO users takes priority over any testing. With regards, Daniel
On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote: > > QEMU probably can't carry OSK key[1] for legal reasons so it > > can't supply the valid default key. However when tests are run > > applesmc will pollute test log with distracting warning, > > silence that warning so it won't distract maintainers/CI. > > What test is causing this problem ? bios-tables-test -- see here for the relevant bit of the log: https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=TkcXrhg@mail.gmail.com/ -- PMM
On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote: > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote: > > > QEMU probably can't carry OSK key[1] for legal reasons so it > > > can't supply the valid default key. However when tests are run > > > applesmc will pollute test log with distracting warning, > > > silence that warning so it won't distract maintainers/CI. > > > > What test is causing this problem ? > > bios-tables-test -- see here for the relevant bit of the log: > > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=TkcXrhg@mail.gmail.com/ The right fix is for bios-tables-tests to pass an explicit 'osk' value then. As its a test it doesn't have to be a genuine OSK, jsut any old 64-byte string diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c index 359916c228..f6b5adf200 100644 --- a/tests/qtest/bios-tables-test.c +++ b/tests/qtest/bios-tables-test.c @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void) .variant = ".applesmc", }; - test_acpi_one("-device isa-applesmc", &data); + test_acpi_one("-device isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr", &data); free_test_data(&data); } With regards, Daniel
On Thu, 28 Jul 2022 at 11:23, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote: > > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote: > > > > QEMU probably can't carry OSK key[1] for legal reasons so it > > > > can't supply the valid default key. However when tests are run > > > > applesmc will pollute test log with distracting warning, > > > > silence that warning so it won't distract maintainers/CI. > > > > > > What test is causing this problem ? > > > > bios-tables-test -- see here for the relevant bit of the log: > > > > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=TkcXrhg@mail.gmail.com/ > > The right fix is for bios-tables-tests to pass an explicit 'osk' value > then. As its a test it doesn't have to be a genuine OSK, jsut any old > 64-byte string > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > index 359916c228..f6b5adf200 100644 > --- a/tests/qtest/bios-tables-test.c > +++ b/tests/qtest/bios-tables-test.c > @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void) > .variant = ".applesmc", > }; > > - test_acpi_one("-device isa-applesmc", &data); > + test_acpi_one("-device isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr", &data); > free_test_data(&data); We should either have a comment saying that this has to be exactly 64 characters and it doesn't matter what they are; or we could use any64characterfakeoskisenoughtopreventinvalidkeywarningsonstderr :-) -- PMM
On Thu, 28 Jul 2022 11:23:00 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote: > > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote: > > > > QEMU probably can't carry OSK key[1] for legal reasons so it > > > > can't supply the valid default key. However when tests are run > > > > applesmc will pollute test log with distracting warning, > > > > silence that warning so it won't distract maintainers/CI. > > > > > > What test is causing this problem ? > > > > bios-tables-test -- see here for the relevant bit of the log: > > > > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=TkcXrhg@mail.gmail.com/ > > The right fix is for bios-tables-tests to pass an explicit 'osk' value > then. As its a test it doesn't have to be a genuine OSK, jsut any old > 64-byte string > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > index 359916c228..f6b5adf200 100644 > --- a/tests/qtest/bios-tables-test.c > +++ b/tests/qtest/bios-tables-test.c > @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void) > .variant = ".applesmc", > }; > > - test_acpi_one("-device isa-applesmc", &data); > + test_acpi_one("-device isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr", &data); > free_test_data(&data); > } that will work, care tho send a formal patch or should I take over? However we still have bogus default_osk, yes it will cause warning which typically nobody will see and end user will still end up with upset guest. Right thing would be to require osk explicitly and drop default completely. Users who actually run MacOS guest must be providing OSK explicitly already so they won't be affected and anyone else using default is broken anyways (whether QEMU started directly or through mgmt layer) > > > With regards, > Daniel
On Thu, 28 Jul 2022 10:48:27 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote: > > QEMU probably can't carry OSK key[1] for legal reasons so it > > can't supply the valid default key. However when tests are run > > applesmc will pollute test log with distracting warning, > > silence that warning so it won't distract maintainers/CI. > > What test is causing this problem ? > > > > > If user launches MacOS guest without supplying valid or any key > > they will get upset MacOS (and legality of specifying valid key > > on CLI is also questionable). This patch favors QEMU/tests vs > > MacOS guest. > > > > A patch from Apple needed to change default to the valid key, > > until then prefer QEMU maintainers sanity/cleaner test logs > > vs any inconvenience users running MacOS guest might suffer. > > > > 1) After some googling, it seems 'secret' key is the same for > > all Mac hardware and is widely known but whether it's > > permited to use that is up to lawyers/Apple to decide. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/misc/applesmc.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c > > index 5f9c742e50..bce27b0950 100644 > > --- a/hw/misc/applesmc.c > > +++ b/hw/misc/applesmc.c > > @@ -84,7 +84,7 @@ enum { > > #define smc_debug(...) do { } while (0) > > #endif > > > > -static char default_osk[64] = "This is a dummy key. Enter the real key " > > +static char default_osk[65] = "This is a dummy key. Enter the real key " > > "using the -osk parameter"; > > This is a distinct bug fix from the problem described. We should > take this as a standalone fix no matter what. thought about it (until I started to write commit message for that) but it becomes bug only if default_osk is treated as NULL terminated string, which happens only if following hunk is applied. Without that, it's treated as 64 bytes. (we can null-terminate it anyways just to be on a safe side) > > struct AppleSMCData { > > @@ -335,10 +335,12 @@ static void applesmc_isa_realize(DeviceState *dev, Error **errp) > > isa_register_ioport(&s->parent_obj, &s->io_err, > > s->iobase + APPLESMC_ERR_PORT); > > > > - if (!s->osk || (strlen(s->osk) != 64)) { > > - warn_report("Using AppleSMC with invalid key"); > > + if (!s->osk) { > > s->osk = default_osk; > > } > > + if (strlen(s->osk) != 64) { > > + warn_report("Using AppleSMC with invalid key"); > > + } > > This means users who invoke QEMU won't see the warning any more. This > feels like a dubious change, as it degrades the user experiance for > sake of tests - IMHO users takes priority over any testing. > > With regards, > Daniel
On Thu, Jul 28, 2022 at 02:00:37PM +0200, Igor Mammedov wrote: > On Thu, 28 Jul 2022 11:23:00 +0100 > Daniel P. Berrangé <berrange@redhat.com> wrote: > > > On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote: > > > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote: > > > > > QEMU probably can't carry OSK key[1] for legal reasons so it > > > > > can't supply the valid default key. However when tests are run > > > > > applesmc will pollute test log with distracting warning, > > > > > silence that warning so it won't distract maintainers/CI. > > > > > > > > What test is causing this problem ? > > > > > > bios-tables-test -- see here for the relevant bit of the log: > > > > > > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=TkcXrhg@mail.gmail.com/ > > > > The right fix is for bios-tables-tests to pass an explicit 'osk' value > > then. As its a test it doesn't have to be a genuine OSK, jsut any old > > 64-byte string > > > > diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > > index 359916c228..f6b5adf200 100644 > > --- a/tests/qtest/bios-tables-test.c > > +++ b/tests/qtest/bios-tables-test.c > > @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void) > > .variant = ".applesmc", > > }; > > > > - test_acpi_one("-device isa-applesmc", &data); > > + test_acpi_one("-device isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr", &data); > > free_test_data(&data); > > } > > that will work, care tho send a formal patch or should I take over? Could you spin something up, as I'm not in a position to do formal testing today. > However we still have bogus default_osk, yes it will cause warning which > typically nobody will see and end user will still end up with upset guest. > Right thing would be to require osk explicitly and drop default completely. > Users who actually run MacOS guest must be providing OSK explicitly already > so they won't be affected and anyone else using default is broken anyways > (whether QEMU started directly or through mgmt layer) There are other patches onlist to make QEMU extract an osk from the host hardware, which is ok because IIUC macOS allows you to run macOS as a VM, provided the host is Apple hardware. With regards, Daniel
Peter Maydell <peter.maydell@linaro.org> writes: > On Thu, 28 Jul 2022 at 11:23, Daniel P. Berrangé <berrange@redhat.com> wrote: >> >> On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote: >> > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé <berrange@redhat.com> wrote: >> > > >> > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote: >> > > > QEMU probably can't carry OSK key[1] for legal reasons so it >> > > > can't supply the valid default key. However when tests are run >> > > > applesmc will pollute test log with distracting warning, >> > > > silence that warning so it won't distract maintainers/CI. >> > > >> > > What test is causing this problem ? >> > >> > bios-tables-test -- see here for the relevant bit of the log: >> > >> > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=TkcXrhg@mail.gmail.com/ >> >> The right fix is for bios-tables-tests to pass an explicit 'osk' value >> then. As its a test it doesn't have to be a genuine OSK, jsut any old >> 64-byte string >> >> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c >> index 359916c228..f6b5adf200 100644 >> --- a/tests/qtest/bios-tables-test.c >> +++ b/tests/qtest/bios-tables-test.c >> @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void) >> .variant = ".applesmc", >> }; >> >> - test_acpi_one("-device isa-applesmc", &data); >> + test_acpi_one("-device isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr", &data); >> free_test_data(&data); > > We should either have a comment saying that this has to be exactly > 64 characters and it doesn't matter what they are; or we could use > any64characterfakeoskisenoughtopreventinvalidkeywarningsonstderr > > :-) I applaud the renaissance of roman-style inscriptions, but it's not just words without spaces, it's also capital letters only: ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR Seriously, throw in some dashes or spaces.
On Thu, 28 Jul 2022 15:29:58 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Thu, 28 Jul 2022 at 11:23, Daniel P. Berrangé <berrange@redhat.com> wrote: > >> > >> On Thu, Jul 28, 2022 at 11:05:13AM +0100, Peter Maydell wrote: > >> > On Thu, 28 Jul 2022 at 10:48, Daniel P. Berrangé <berrange@redhat.com> wrote: > >> > > > >> > > On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote: > >> > > > QEMU probably can't carry OSK key[1] for legal reasons so it > >> > > > can't supply the valid default key. However when tests are run > >> > > > applesmc will pollute test log with distracting warning, > >> > > > silence that warning so it won't distract maintainers/CI. > >> > > > >> > > What test is causing this problem ? > >> > > >> > bios-tables-test -- see here for the relevant bit of the log: > >> > > >> > https://lore.kernel.org/qemu-devel/CAFEAcA8u8jm7b+JD_t0qMNMy+WSJPOw=qxqptZpwTp=TkcXrhg@mail.gmail.com/ > >> > >> The right fix is for bios-tables-tests to pass an explicit 'osk' value > >> then. As its a test it doesn't have to be a genuine OSK, jsut any old > >> 64-byte string > >> > >> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c > >> index 359916c228..f6b5adf200 100644 > >> --- a/tests/qtest/bios-tables-test.c > >> +++ b/tests/qtest/bios-tables-test.c > >> @@ -1632,7 +1632,7 @@ static void test_acpi_q35_applesmc(void) > >> .variant = ".applesmc", > >> }; > >> > >> - test_acpi_one("-device isa-applesmc", &data); > >> + test_acpi_one("-device isa-applesmc,osk=iamalsonottherealoskimjustheretostopbiostablestestspammingstderr", &data); > >> free_test_data(&data); > > > > We should either have a comment saying that this has to be exactly > > 64 characters and it doesn't matter what they are; or we could use > > any64characterfakeoskisenoughtopreventinvalidkeywarningsonstderr > > > > :-) > > I applaud the renaissance of roman-style inscriptions, but it's not just > words without spaces, it's also capital letters only: > > ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR > > Seriously, throw in some dashes or spaces. too late, but added a comment above hieroglyphs style string >
On Thu, 28 Jul 2022 at 14:30, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > I applaud the renaissance of roman-style inscriptions, but it's not just > words without spaces, it's also capital letters only: > > ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR > > Seriously, throw in some dashes or spaces. any-64-char-fake-osk-will-avoid-an-invalid-key-warning-on-stderr -- PMM
On Thu, Jul 28, 2022 at 02:40:22PM +0100, Peter Maydell wrote: > On Thu, 28 Jul 2022 at 14:30, Markus Armbruster <armbru@redhat.com> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > I applaud the renaissance of roman-style inscriptions, but it's not just > > words without spaces, it's also capital letters only: > > > > ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR > > > > Seriously, throw in some dashes or spaces. > > any-64-char-fake-osk-will-avoid-an-invalid-key-warning-on-stderr On the basis that virtualization gives you turtles all the way down... -device isa-applesmc,osk=🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢 With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Thu, Jul 28, 2022 at 02:40:22PM +0100, Peter Maydell wrote: >> On Thu, 28 Jul 2022 at 14:30, Markus Armbruster <armbru@redhat.com> wrote: >> > Peter Maydell <peter.maydell@linaro.org> writes: >> > I applaud the renaissance of roman-style inscriptions, but it's not just >> > words without spaces, it's also capital letters only: >> > >> > ANY64CHARACTERFAKEOSKISENOUGHTOPREVENTINVALIDKEYWARNINGSONSTDERR >> > >> > Seriously, throw in some dashes or spaces. >> >> any-64-char-fake-osk-will-avoid-an-invalid-key-warning-on-stderr > > On the basis that virtualization gives you turtles all the way down... > > -device isa-applesmc,osk=🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢🐢 Oh, Egyptian rather than Roman. Works for me!
diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c index 5f9c742e50..bce27b0950 100644 --- a/hw/misc/applesmc.c +++ b/hw/misc/applesmc.c @@ -84,7 +84,7 @@ enum { #define smc_debug(...) do { } while (0) #endif -static char default_osk[64] = "This is a dummy key. Enter the real key " +static char default_osk[65] = "This is a dummy key. Enter the real key " "using the -osk parameter"; struct AppleSMCData { @@ -335,10 +335,12 @@ static void applesmc_isa_realize(DeviceState *dev, Error **errp) isa_register_ioport(&s->parent_obj, &s->io_err, s->iobase + APPLESMC_ERR_PORT); - if (!s->osk || (strlen(s->osk) != 64)) { - warn_report("Using AppleSMC with invalid key"); + if (!s->osk) { s->osk = default_osk; } + if (strlen(s->osk) != 64) { + warn_report("Using AppleSMC with invalid key"); + } QLIST_INIT(&s->data_def); qdev_applesmc_isa_reset(dev);
QEMU probably can't carry OSK key[1] for legal reasons so it can't supply the valid default key. However when tests are run applesmc will pollute test log with distracting warning, silence that warning so it won't distract maintainers/CI. If user launches MacOS guest without supplying valid or any key they will get upset MacOS (and legality of specifying valid key on CLI is also questionable). This patch favors QEMU/tests vs MacOS guest. A patch from Apple needed to change default to the valid key, until then prefer QEMU maintainers sanity/cleaner test logs vs any inconvenience users running MacOS guest might suffer. 1) After some googling, it seems 'secret' key is the same for all Mac hardware and is widely known but whether it's permited to use that is up to lawyers/Apple to decide. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/misc/applesmc.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)