Re: [Qemu-devel] [PATCH v2] fw_cfg: RFQDN rules, documentation

[Gabriel L. Somlo]

On Thu, Apr 07, 2016 at 07:40:12PM +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 07, 2016 at 06:23:24PM +0200, Laszlo Ersek wrote:
> > On 04/07/16 17:38, Michael S. Tsirkin wrote:
> > > This requires that all -fw_cfg command line users use names of the form
> > > opt/RFQDN/: such names are compatible with QEMU 2.4 and 2.5 as well as
> > > future QEMU versions.
> > > 
> > > As ability to insert fw_cfg entries in QEMU root is useful for
> > > firmware development, add a special prefix: unsupported/root/ that
> > > allows that, while making sure users are aware it's unsupported.
> > > 
> > > Cc: Gerd Hoffmann <address@hidden>
> > > Cc: Gabriel L. Somlo <address@hidden>
> > > Cc: Laszlo Ersek <address@hidden>
> > > Cc: Markus Armbruster <address@hidden>
> > > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > > ---
> > > 
> > > changes from v1:
> > >     address comments by Laszlo Ersek.
> > > 
> > > There are still things worrying me
> > > 
> > > 1. there is apparently no way to tell linux guests whether it should 
> > > expose
> > >    a specific file to userspace.
> > >
> > > 2. Should we have opt/fw/ or opt/hidden/ for firmware use?
> > >    Alternatively, agree to hide files and/or directories
> > >    starting with e.g. "."?
> > 
> > Hm, is #2 an idea for addressing #1?
> > 
> > For interpreting #2, I again have to reach back to the three groups of
> > people you identified -- QEMU developers, QEMU firmware developers, and
> > users.
> > 
> > Since you say "for firmware use", I guess the point would be to enable
> > QEMU firmware developers to create such settings, either for
> > (a) population by QEMU, or for
> > (b) population by firmware end-users,
> > that the guest kernel would be prevented from seeing.
> > 
> > Furthermore, since your examples both start with opt/, *and* we have
> > language saying
> > 
> >   QEMU developers MUST NOT use item names prefixed with "opt/" when
> >   inserting items programmatically
> > 
> > I determine that option (a) must not be your intent. Therefore, the
> > question is, I think:
> > 
> >   Should we allow QEMU firmware developers to create special settings,
> >   to be populated manually by their end-users, that the guest kernel
> >   would be prevented from seeing?
> 
> Exactly.
> 
> > I don't think so. Namely, in practice, new firmware settings (that are
> > to be populated manually by users) will go under "opt/org.seabios/" and
> > "opt/org.tianocore.edk2.ovmf/". I couldn't care less if a guest kernel
> > user looks at such files. After all, the names *explicitly carry* the
> > RFQDN of the intended consumer. If a user violates it, that's his
> > problem. (It may become the problem of his downstream users too, but
> > that's the same thing.)
> > 
> > So, as long as I understood your question right, I don't think it's
> > necessary.
> 
> It's not a question we need to ask ourselves as hardware/qemu designers.
> It's a question for the guest kernel - once that exposes
> interfaces to applications, it has to maintain them forever.

And that's why IMHO it's cleaner for that interface to be:

        /sys/firmware/qemu-fw-cfg/by-name/<blob-path>/[key|name|raw|size]

I really don't think any particular instance of <blob-path> could
reasonably be called an "interface" (and therefore create expectations
of its continued presence forever), or can it ?

Thanks,
--Gabriel

> This is unlike firmware interfaces - if these are updated
> together with firmware, you do not need to maintain
> old ones.
> 
> > I have one other comment below:
> > 
> > >  vl.c                  | 44 ++++++++++++++++++++++++++++++++++++++++----
> > >  docs/specs/fw_cfg.txt | 34 +++++++++++++++++-----------------
> > >  qemu-options.hx       | 38 +++++++++++++++++++++++++++++++++-----
> > >  3 files changed, 90 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index 2200e62..aec8a94 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -2296,8 +2296,11 @@ static int parse_fw_cfg(void *opaque, QemuOpts 
> > > *opts, Error **errp)
> > >  {
> > >      gchar *buf;
> > >      size_t size;
> > > -    const char *name, *file, *str;
> > > +    const char *name, *file, *str, *slash, *dot;
> > >      FWCfgState *fw_cfg = (FWCfgState *) opaque;
> > > +    static const char qemu_prefix[] = "opt/org.qemu";
> > > +    static const char ovmf_prefix[] = "opt/ovmf/";
> > > +    static const char unsupported_root_prefix[] = "unsupported/root/";
> > >  
> > >      if (fw_cfg == NULL) {
> > >          error_report("fw_cfg device not available");
> > > @@ -2320,9 +2323,42 @@ static int parse_fw_cfg(void *opaque, QemuOpts 
> > > *opts, Error **errp)
> > >          error_report("name too long (max. %d char)", 
> > > FW_CFG_MAX_FILE_PATH - 1);
> > >          return -1;
> > >      }
> > > -    if (strncmp(name, "opt/", 4) != 0) {
> > > -        error_report("warning: externally provided fw_cfg item names "
> > > -                     "should be prefixed with \"opt/\"");
> > > +    /*
> > > +     * Look for and strip unsupported_root_prefix, which is useful for 
> > > firmware
> > > +     * development, but warn users.
> > > +     */
> > > +    if (!strncmp(name, unsupported_root_prefix,
> > > +                 sizeof(unsupported_root_prefix) - 1)) {
> > > +        error_report("warning: removing prefix \"%s\". "
> > > +                     "Guest or QEMU may crash. "
> > > +                     "Names must be prefixed with \"opt/RFQDN/\"",
> > > +                     unsupported_root_prefix);
> > > +        name += strlen(unsupported_root_prefix);
> > 
> > I think here you missed my separate comment about the sizeof
> > replacement. I'm not insisting on it, of course, but in v2 you did
> > replace all other strlen()s with sizeof, so I think this was an oversight.
> > 
> > If you fix it:
> > 
> > Reviewed-by: Laszlo Ersek <address@hidden>
> > 
> > Thanks
> > Laszlo
> > 
> > > +        if (!nonempty_str(name)) {
> > > +            error_report("invalid argument(s)");
> > > +            return -1;
> > > +        }
> > > +    } else if (!strncmp(name, ovmf_prefix, sizeof(ovmf_prefix) - 1)) {
> > > +        /* Allow the prefix used historically with ovmf. */
> > > +    } else {
> > > +        /*
> > > +         * Don't attempt to validate a valid RFQDN in name, as that's 
> > > not easy:
> > > +         * we do validate that it includes '.' .
> > > +         */
> > > +        if (strncmp(name, "opt/", 4) ||
> > > +            !(dot = strchr(name + 4, '.')) ||
> > > +            !(slash = strchr(name + 4, '/')) ||
> > > +            dot > slash) {
> > > +            error_report("error: externally provided fw_cfg item names "
> > > +                         "must be prefixed with \"opt/RFQDN/\"");
> > > +            return -1;
> > > +        }
> > > +        if (!strncmp(name, qemu_prefix, sizeof(qemu_prefix) - 1)) {
> > > +            error_report("error: externally provided fw_cfg item names "
> > > +                         "must not use the reserved prefix \"%s\"",
> > > +                         qemu_prefix);
> > > +            return -1;
> > > +        }
> > >      }
> > >      if (nonempty_str(str)) {
> > >          size = strlen(str); /* NUL terminator NOT included in fw_cfg 
> > > blob */
> > > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> > > index 5414140..41ce9ca 100644
> > > --- a/docs/specs/fw_cfg.txt
> > > +++ b/docs/specs/fw_cfg.txt
> > > @@ -210,29 +210,29 @@ the following syntax:
> > >  
> > >      -fw_cfg [name=]<item_name>,file=<path>
> > >  
> > > -where <item_name> is the fw_cfg item name, and <path> is the location
> > > -on the host file system of a file containing the data to be inserted.
> > > -
> > > -Small enough items may be provided directly as strings on the command
> > > -line, using the syntax:
> > > +Or
> > >  
> > >      -fw_cfg [name=]<item_name>,string=<string>
> > >  
> > > -The terminating NUL character of the content <string> will NOT be
> > > -included as part of the fw_cfg item data, which is consistent with
> > > -the absence of a NUL terminator for items inserted via the file option.
> > > +See QEMU man page for more documentation.
> > >  
> > > -Both <item_name> and, if applicable, the content <string> are passed
> > > -through by QEMU without any interpretation, expansion, or further
> > > -processing. Any such processing (potentially performed e.g., by the 
> > > shell)
> > > -is outside of QEMU's responsibility; as such, using plain ASCII 
> > > characters
> > > -is recommended.
> > > +Using item_name with plain ASCII characters only is recommended.
> > >  
> > > -NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/"
> > > +Users MUST choose item names beginning with the prefix "opt/RFQDN/"
> > >  when using the "-fw_cfg" command line option, to avoid conflicting with
> > > -item names used internally by QEMU. For instance:
> > > +item names used internally by QEMU, or by firmware. For instance:
> > >  
> > > -    -fw_cfg name=opt/my_item_name,file=./my_blob.bin
> > > +    -fw_cfg 
> > > name=opt/com.mycompany/guestagent/guestblob,file=./my_blob.bin
> > >  
> > > -Similarly, QEMU developers *SHOULD NOT* use item names prefixed with
> > > +Similarly, QEMU developers MUST NOT use item names prefixed with
> > >  "opt/" when inserting items programmatically, e.g. via fw_cfg_add_file().
> > > +
> > > +For historical reasons "opt/ovmf/" is reserved for use with the OVMF 
> > > firmware.
> > > +
> > > +To simplify guest firmware development, the prefix
> > > +unsupported/root/ is automatically stripped from paths, which
> > > +allows creating fw_cfg files in the root QEMU directory.  This interface 
> > > is
> > > +strictly for use by developers, its use can cause guest or QEMU crashes, 
> > > is
> > > +unsupported and can be removed at any point.
> > > +
> > > +Any use of the prefix "opt/org.qemu" is reserved for future use.
> > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > index a770086..a5abf98 100644
> > > --- a/qemu-options.hx
> > > +++ b/qemu-options.hx
> > > @@ -2860,18 +2860,46 @@ ETEXI
> > >  
> > >  DEF("fw_cfg", HAS_ARG, QEMU_OPTION_fwcfg,
> > >      "-fw_cfg [name=]<name>,file=<file>\n"
> > > -    "                add named fw_cfg entry from file\n"
> > > +    "                add named fw_cfg entry with content from file\n"
> > >      "-fw_cfg [name=]<name>,string=<str>\n"
> > > -    "                add named fw_cfg entry from string\n",
> > > +    "                add named fw_cfg entry with content from string\n",
> > >      QEMU_ARCH_ALL)
> > >  STEXI
> > > +
> > >  @item -fw_cfg address@hidden,address@hidden
> > >  @findex -fw_cfg
> > > -Add named fw_cfg entry from file. @var{name} determines the name of
> > > -the entry in the fw_cfg file directory exposed to the guest.
> > > +Add named fw_cfg entry with content from file @var{file}.
> > >  
> > >  @item -fw_cfg address@hidden,address@hidden
> > > -Add named fw_cfg entry from string.
> > > +Add named fw_cfg entry with content from string @var{str}, up to the 
> > > first NUL character.
> > > +
> > > +The terminating NUL character of the content @var{str} will NOT be
> > > +included as part of the fw_cfg item data. To insert content including
> > > +the NUL character, store it in file and insert the item via
> > > +the @var{file} option.
> > > +
> > > +Both the name and the content are passed by QEMU through to the guest, 
> > > where:
> > > address@hidden @option
> > > address@hidden @var{name} determines the name of the entry in the fw_cfg 
> > > file directory exposed to the guest.
> > > +
> > > address@hidden must be in the format opt/RFQDN/<item_name>.
> > > +
> > > +Any processing of @var{name} values (potentially performed e.g., by the 
> > > shell)
> > > +is outside of QEMU's responsibility; as such, using plain ASCII 
> > > characters is
> > > +recommended.
> > > address@hidden table
> > > +
> > > +Example:
> > > address@hidden
> > > +    -fw_cfg 
> > > name=opt/com.mycompany/guestagent/guestblob,file=./my_blob.bin
> > > address@hidden example
> > > +
> > > +To simplify guest firmware development, the prefix
> > > +unsupported/root/ is automatically stripped from paths, which
> > > +allows creating fw_cfg files in the root QEMU directory.  This interface 
> > > is
> > > +strictly for use by developers, its use can cause guest or QEMU crashes, 
> > > is
> > > +unsupported and can be removed at any point.
> > > +
> > >  ETEXI
> > >  
> > >  DEF("serial", HAS_ARG, QEMU_OPTION_serial, \
> > >