[PATCH v2 4/8] qemu: Validate firmware blob configuration

[Daniel P. Berrangé]

On Thu, Jun 04, 2020 at 08:44:05PM +0200, Michal Privoznik wrote:
> There are recommendations and limitations to the name of the
> config blobs we need to follow [1].
> 
> Firstly, we don't want users to change any value only add new
> blobs. This means, that the name must have "opt/" prefix and at
> the same time must not begin with "opt/ovmf" nor "opt/org.qemu"
> as these are reserved for OVMF or QEMU respectively.
> 
> Secondly, there is a limit (FW_CFG_MAX_FILE_PATH in qemu.git) of
> 56 characters for filename.

Ewww, that is horrible. I'm have inclined to say we should leave the
limit unchecked in libvirt, and file a BZ against QEMU. It should be
using g_strdup_printf() with filenames and not allocating on the
stack. We already see peoiple exceeding the 100 charater limit of UNIX
sockets, so a 56 character limit is going to be trivially exceeded
without even trying hard.

> 1: docs/specs/fw_cfg.txt from qemu.git
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_validate.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 584d1375b8..56a7ebfd7f 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -762,6 +762,41 @@ qemuValidateDefGetVcpuHotplugGranularity(const virDomainDef *def)
>  }
>  
>  
> +#define QEMU_FW_CFG_MAX_FILE_PATH 55
> +static int
> +qemuValidateDomainDefSysinfo(const virSysinfoDef *def,
> +                             virQEMUCapsPtr qemuCaps G_GNUC_UNUSED)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < def->nfw_cfgs; i++) {
> +        const virSysinfoFWCfgDef *f = &def->fw_cfgs[i];
> +
> +        if (!STRPREFIX(f->name, "opt/")) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("Invalid firmware name"));
> +            return -1;
> +        }
> +
> +        if (STRPREFIX(f->name, "opt/ovmf/") ||
> +            STRPREFIX(f->name, "opt/org.qemu/")) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("That firmware name is reserved"));
> +            return -1;
> +        }
> +
> +        if (f->file &&
> +            strlen(f->file) > QEMU_FW_CFG_MAX_FILE_PATH) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("firmware file too long"));
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
>  int
>  qemuValidateDomainDef(const virDomainDef *def,
>                        void *opaque)
> @@ -978,6 +1013,11 @@ qemuValidateDomainDef(const virDomainDef *def,
>          }
>      }
>  
> +    for (i = 0; i < def->nsysinfo; i++) {
> +        if (qemuValidateDomainDefSysinfo(def->sysinfo[i], qemuCaps) < 0)
> +            return -1;
> +    }
> +
>      return 0;
>  }
>  
> -- 
> 2.26.2
> 

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 :|