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.
1: docs/specs/fw_cfg.txt from qemu.git
Signed-off-by: Michal Privoznik <mprivozn@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
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@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 :|
On 6/9/20 11:52 AM, Daniel P. Berrangé wrote: > 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. Ah, I got it wrong when reading the documentation. The limit is the name of the entry, not file name associated. For instance the following works: -fw_cfg name=opt/com.example/blah,file=/tmp/some_very_long_path_that_is_more_than_fifty_six_characters_long_to_see_what_happens_if_I_do_that but if @name would be too long QEMU would fail: qemu-system-x86_64: -fw_cfg name=opt/com.example/some_very_long_...,file=...: name too long (max. 55 char) And this comes from kernel's implementation of qemu_fw_cfg. However, the name can be up to PAGE_SIZE long (minus 2 for terminating \n and NUL), according to sysfs documentation (Documentation/filesystems/sysfs.rst:241): - The buffer will always be PAGE_SIZE bytes in length. On i386, this is 4096. Given this new realization, I think I'll just remove the check and not fill bug. I don't think we need longer names, do we? Michal
On Tue, Jun 09, 2020 at 04:44:46PM +0200, Michal Privoznik wrote: > On 6/9/20 11:52 AM, Daniel P. Berrangé wrote: > > 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. > > Ah, I got it wrong when reading the documentation. The limit is the name of > the entry, not file name associated. For instance the following works: > > -fw_cfg name=opt/com.example/blah,file=/tmp/some_very_long_path_that_is_more_than_fifty_six_characters_long_to_see_what_happens_if_I_do_that > > but if @name would be too long QEMU would fail: > > qemu-system-x86_64: -fw_cfg > name=opt/com.example/some_very_long_...,file=...: name too long (max. 55 > char) > > And this comes from kernel's implementation of qemu_fw_cfg. However, the > name can be up to PAGE_SIZE long (minus 2 for terminating \n and NUL), > according to sysfs documentation (Documentation/filesystems/sysfs.rst:241): > > - The buffer will always be PAGE_SIZE bytes in length. On i386, this > is 4096. > > Given this new realization, I think I'll just remove the check and not fill > bug. I don't think we need longer names, do we? Yep, fine with me. 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 :|
© 2016 - 2026 Red Hat, Inc.