Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used

[Igor Mammedov <imammedo@redhat.com>]

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