Re: [Qemu-devel] [PATCH v2] vl.c: disallow command line fw cfg without opt/

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Mar 16, 2016 at 08:15:07PM +0100, Laszlo Ersek wrote:
> On 03/16/16 19:43, Michael S. Tsirkin wrote:
> > On Wed, Mar 16, 2016 at 07:35:09PM +0100, Laszlo Ersek wrote:
> >> On 03/16/16 19:15, Gabriel L. Somlo wrote:
> >>> On Wed, 16 Mar 2016 at 18:50:57 +0200, Michael S. Tsirkin wrote:
> >>>> On Wed, Mar 16, 2016 at 05:29:45PM +0100, Markus Armbruster wrote:
> >>>>> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >>>>>
> >>>>>> Allowing arbitary file names on command line is setting us up for
> >>>>>> failure: future guests will look for a specific QEMU-specified name and
> >>>>>> will get confused finding a user file there.
> >>>>>>
> >>>>>> We do warn but people are conditioned to ignore warnings by now,
> >>>>>> so at best that will help users debug problem, not avoid it.
> >>>>>>
> >>>>>> Disable this by default, so distros don't get to deal with it,
> >>>>>> but leave an option for developers to configure this in,
> >>>>>> with scary warnings so people only do it if they know
> >>>>>> what they are doing.
> >>>>>>
> >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>>
> >>>>> I'm having a hard time to see the point.
> >>>>
> >>>> Frankly, I am having a hard time to see the point of exposing fw cfg to
> >>>> users at all.  It was designed as an internal interface between QEMU PC
> >>>> hardware and firmware.  As a PC maintainer, I do not like it that users
> >>>> get to poke at PC internals.
> >>>>
> >>>> So it is yet another way to pass binaries to Linux guests.  Don't we
> >>>> have enough of these?  But Gerd likes it for some reason, and merged it.
> >>>> OK.
> >>>
> >>> As the author of the feature, I feel I should jump back in and clarify:
> >>>
> >>> It's a way to pass arbitrary blobs to any type of guest, with two
> >>> important properties: 1. asynchronous, and 2. out-of-band. When I
> >>> started looking, all existing methods involved either having the host
> >>> start polling for the guest to bring up qga and be ready to accept an
> >>> out-of-band connection (i.e., *not* asynchronous), or have the guest
> >>> mount some special cdrom or floppy image prepared by the host (i.e.,
> >>> *not* out of band).
> >>>
> >>> fw_cfg is both asynchronous and out-of-band, so it appeared to be the
> >>> perfect choice.
> >>>
> >>>> But please find a way to make sure it does not conflict with its current
> >>>> usage in PC.  Asking that all files have an "opt/" prefix is one way
> >>>> but only if it is enforced.
> >>>
> >>> Enforcing the "opt/" prefix was clearly on the table when I submitted
> >>> the feature (and totally acceptable for my own needs). At the time, however,
> >>> most of the advice I received on the list was to leave it as a warning
> >>> only (i.e., "mechanism, not policy"), especially since other respondents
> >>> expressed interest in passing in non-"/opt" blobs for easier development
> >>> and debugging of alternative firmware (such as OVMF, iirc).
> >>>
> >>> Having a mis-use of this feature become "institutionalized" over time was
> >>> seen as a low/negligible risk at the time. Do we have any new reasons
> >>> to worry about it ?
> >>
> >> OVMF uses this feature for a few flags. They are all called
> >> "opt/ovmf/...". I followed the advice in "docs/specs/fw_cfg.txt" (which
> >> shouldn't be surprising since I seem to have reviewed every patch for
> >> that file):
> >>
> >>> NOTE: Users *SHOULD* choose item names beginning with the prefix "opt/"
> >>> when using the "-fw_cfg" command line option, to avoid conflicting with
> >>> item names used internally by QEMU. For instance:
> >>>
> >>> -fw_cfg name=opt/my_item_name,file=./my_blob.bin
> >>>
> >>> Similarly, QEMU developers *SHOULD NOT* use item names prefixed with
> >>> "opt/" when inserting items programmatically, e.g. via fw_cfg_add_file().
> >>
> >> It says "should", not "must".
> > 
> > should means "might be ok".
> 
> Yes, if there is a pressing reason. And even then, "you have been warned".
> 
> > So change it to must then?
> 
> I didn't suggest that.
> 
> (For consistency, your patch should be attempting to modify the code and
> the docs together -- but this doesn't mean that I agree with the patch.)
> 
> >> I liked (and like) the "mechanism, not
> >> policy" thing. Letting developers pass in whatever they want, for
> >> development / debugging / testing purposes, is a plus to me.
> >>
> >> Thanks
> >> Laszlo
> > 
> > Could you flesh out the development / debugging / testing and
> > how is inserting files in PC namespace useful for that?
> 
> I don't know -- which is part of the argument. Lack of a ready example
> doesn't imply (to me) that the possibility should be excluded.
> 
> As Paolo said, I believe we've been through this discussion. I have no
> new arguments; the old ones are valid to me still. I won't try to
> influence this discussion any further; I only chimed in because OVMF was
> mentioned (and because I noticed that the docs would get out of sync
> with the code).
> 
> Thanks
> Laszlo

By the way, these are developer docs.  User docs do not say anything:

       -fw_cfg [name=]name,file=file
           Add named fw_cfg entry from file. name determines the name of
	   the entry in the fw_cfg file directory exposed to the guest.

       -fw_cfg [name=]name,string=str
           Add named fw_cfg entry from string.


-- 
MST