From 8c75de1966e4e5babf4faf0e86209280220d8dbe Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Wed, 13 May 2020 16:58:55 +0200 Subject: [PATCH 01/24] config: Add 'List' alternates for hypervisor configuration paths Paths mentioned in the hypervisor configuration can be overriden using annotations, which is potentially dangerous. For each path, add a 'List' variant that specifies the list of acceptable values from annotations. Bug: https://bugs.launchpad.net/katacontainers.io/+bug/1878234 Fixes: #901 Signed-off-by: Christophe de Dinechin --- src/runtime/pkg/katautils/config.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/runtime/pkg/katautils/config.go b/src/runtime/pkg/katautils/config.go index 5a7680920d..a3949d559a 100644 --- a/src/runtime/pkg/katautils/config.go +++ b/src/runtime/pkg/katautils/config.go @@ -71,9 +71,12 @@ type factory struct { type hypervisor struct { Path string `toml:"path"` + PathList []string `toml:"path_list"` JailerPath string `toml:"jailer_path"` + JailerPathList []string `toml:"jailer_path_list"` Kernel string `toml:"kernel"` CtlPath string `toml:"ctlpath"` + CtlPathList []string `toml:"ctlpath_list"` Initrd string `toml:"initrd"` Image string `toml:"image"` Firmware string `toml:"firmware"` @@ -85,6 +88,7 @@ type hypervisor struct { EntropySource string `toml:"entropy_source"` SharedFS string `toml:"shared_fs"` VirtioFSDaemon string `toml:"virtio_fs_daemon"` + VirtioFSDaemonList []string `toml:"virtio_fs_daemon_list"` VirtioFSCache string `toml:"virtio_fs_cache"` VirtioFSExtraArgs []string `toml:"virtio_fs_extra_args"` VirtioFSCacheSize uint32 `toml:"virtio_fs_cache_size"` @@ -93,6 +97,7 @@ type hypervisor struct { BlockDeviceCacheNoflush bool `toml:"block_device_cache_noflush"` EnableVhostUserStore bool `toml:"enable_vhost_user_store"` VhostUserStorePath string `toml:"vhost_user_store_path"` + VhostUserStorePathList []string `toml:"vhost_user_store_path_list"` NumVCPUs int32 `toml:"default_vcpus"` DefaultMaxVCPUs uint32 `toml:"default_maxvcpus"` MemorySize uint32 `toml:"default_memory"` @@ -108,6 +113,7 @@ type hypervisor struct { IOMMU bool `toml:"enable_iommu"` IOMMUPlatform bool `toml:"enable_iommu_platform"` FileBackedMemRootDir string `toml:"file_mem_backend"` + FileBackedMemRootList []string `toml:"file_mem_backend_list"` Swap bool `toml:"enable_swap"` Debug bool `toml:"enable_debug"` DisableNestingChecks bool `toml:"disable_nesting_checks"` @@ -647,6 +653,7 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { DisableBlockDeviceUse: h.DisableBlockDeviceUse, SharedFS: sharedFS, VirtioFSDaemon: h.VirtioFSDaemon, + VirtioFSDaemonList: h.VirtioFSDaemonList, VirtioFSCacheSize: h.VirtioFSCacheSize, VirtioFSCache: h.defaultVirtioFSCache(), VirtioFSExtraArgs: h.VirtioFSExtraArgs, From bf13ff0a3ab314e30257b9b5c352a3b8d5f50776 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 14 May 2020 20:18:18 +0200 Subject: [PATCH 02/24] config: Protect virtio_fs_daemon annotation Sending the virtio_fs_daemon annotation can be used to execute arbitrary code on the host. In order to prevent this, restrict the values of the annotation to a list provided by the configuration file. Fixes: #901 Signed-off-by: Christophe de Dinechin --- .../cli/config/configuration-clh.toml.in | 3 ++ .../configuration-qemu-virtiofs.toml.in | 5 ++- .../cli/config/configuration-qemu.toml.in | 17 +++++--- src/runtime/pkg/katautils/config.go | 7 ++- src/runtime/virtcontainers/hypervisor.go | 6 +++ src/runtime/virtcontainers/persist.go | 4 ++ .../virtcontainers/persist/api/config.go | 6 +++ src/runtime/virtcontainers/pkg/oci/utils.go | 29 +++++++++---- .../virtcontainers/pkg/oci/utils_test.go | 43 +++++++++++++++---- 9 files changed, 94 insertions(+), 26 deletions(-) diff --git a/src/runtime/cli/config/configuration-clh.toml.in b/src/runtime/cli/config/configuration-clh.toml.in index d4d4e7978d..b3a74705ac 100644 --- a/src/runtime/cli/config/configuration-clh.toml.in +++ b/src/runtime/cli/config/configuration-clh.toml.in @@ -62,6 +62,9 @@ default_memory = @DEFMEMSZ@ # Path to vhost-user-fs daemon. virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" +# List of valid annotations values for the virtiofs daemon (default: empty) +# virtio_fs_daemon_list = [ "/opt/kata/bin/virtiofsd", "/usr/.*/virtiofsd" ] + # Default size of DAX cache in MiB virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ diff --git a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in index a199bd30df..4a1523b439 100644 --- a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in +++ b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in @@ -110,6 +110,9 @@ shared_fs = "@DEFSHAREDFS_QEMU_VIRTIOFS@" # Path to vhost-user-fs daemon. virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" +# List of valid annotations values for the virtiofs daemon (default: empty) +# virtio_fs_daemon_list = [ "/opt/kata/bin/virtiofsd", "/usr/.*/virtiofsd" ] + # Default size of DAX cache in MiB virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ @@ -238,7 +241,7 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" #hotplug_vfio_on_root_bus = true # If vhost-net backend for virtio-net is not desired, set to true. Default is false, which trades off -# security (vhost-net runs ring0) for network I/O performance. +# security (vhost-net runs ring0) for network I/O performance. #disable_vhost_net = true # diff --git a/src/runtime/cli/config/configuration-qemu.toml.in b/src/runtime/cli/config/configuration-qemu.toml.in index 0da8d50666..22054c0374 100644 --- a/src/runtime/cli/config/configuration-qemu.toml.in +++ b/src/runtime/cli/config/configuration-qemu.toml.in @@ -101,10 +101,10 @@ default_memory = @DEFMEMSZ@ #enable_virtio_mem = true # Disable block device from being used for a container's rootfs. -# In case of a storage driver like devicemapper where a container's +# In case of a storage driver like devicemapper where a container's # root file system is backed by a block device, the block device is passed -# directly to the hypervisor for performance reasons. -# This flag prevents the block device from being passed to the hypervisor, +# directly to the hypervisor for performance reasons. +# This flag prevents the block device from being passed to the hypervisor, # 9pfs is used instead to pass the rootfs. disable_block_device_use = @DEFDISABLEBLOCK@ @@ -116,6 +116,9 @@ shared_fs = "@DEFSHAREDFS@" # Path to vhost-user-fs daemon. virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" +# List of valid annotations values for the virtiofs daemon (default: empty) +# virtio_fs_daemon_list = [ "/opt/kata/bin/virtiofsd", "/usr/.*/virtiofsd" ] + # Default size of DAX cache in MiB virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ @@ -180,7 +183,7 @@ enable_iothreads = @DEFENABLEIOTHREADS@ # Enabling this will result in the VM memory # being allocated using huge pages. # This is useful when you want to use vhost-user network -# stacks within the container. This will automatically +# stacks within the container. This will automatically # result in memory pre allocation #enable_hugepages = true @@ -236,9 +239,9 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # Default is false #disable_image_nvdimm = true -# VFIO devices are hotplugged on a bridge by default. +# VFIO devices are hotplugged on a bridge by default. # Enable hotplugging on root bus. This may be required for devices with -# a large PCI bar, as this is a current limitation with hotplugging on +# a large PCI bar, as this is a current limitation with hotplugging on # a bridge. This value is valid for "pc" machine type. # Default false #hotplug_vfio_on_root_bus = true @@ -251,7 +254,7 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" #pcie_root_port = 2 # If vhost-net backend for virtio-net is not desired, set to true. Default is false, which trades off -# security (vhost-net runs ring0) for network I/O performance. +# security (vhost-net runs ring0) for network I/O performance. #disable_vhost_net = true # diff --git a/src/runtime/pkg/katautils/config.go b/src/runtime/pkg/katautils/config.go index a3949d559a..6bf59090e8 100644 --- a/src/runtime/pkg/katautils/config.go +++ b/src/runtime/pkg/katautils/config.go @@ -71,7 +71,7 @@ type factory struct { type hypervisor struct { Path string `toml:"path"` - PathList []string `toml:"path_list"` + HypervisorPathList []string `toml:"path_list"` JailerPath string `toml:"jailer_path"` JailerPathList []string `toml:"jailer_path_list"` Kernel string `toml:"kernel"` @@ -533,6 +533,7 @@ func newFirecrackerHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{ HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, JailerPath: jailer, KernelPath: kernel, InitrdPath: initrd, @@ -634,6 +635,7 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{ HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, KernelPath: kernel, InitrdPath: initrd, ImagePath: image, @@ -723,6 +725,7 @@ func newAcrnHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{ HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, KernelPath: kernel, ImagePath: image, HypervisorCtlPath: hypervisorctl, @@ -793,6 +796,7 @@ func newClhHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{ HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, KernelPath: kernel, InitrdPath: initrd, ImagePath: image, @@ -811,6 +815,7 @@ func newClhHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { DisableBlockDeviceUse: h.DisableBlockDeviceUse, SharedFS: sharedFS, VirtioFSDaemon: h.VirtioFSDaemon, + VirtioFSDaemonList: h.VirtioFSDaemonList, VirtioFSCacheSize: h.VirtioFSCacheSize, VirtioFSCache: h.VirtioFSCache, MemPrealloc: h.MemPrealloc, diff --git a/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index cf1a53b700..525133d59a 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/virtcontainers/hypervisor.go @@ -275,6 +275,9 @@ type HypervisorConfig struct { // HypervisorPath is the hypervisor executable host path. HypervisorPath string + // HypervisorPathList is the list of hypervisor paths names allowed in annotations + HypervisorPathList []string + // HypervisorCtlPath is the hypervisor ctl executable host path. HypervisorCtlPath string @@ -309,6 +312,9 @@ type HypervisorConfig struct { // VirtioFSDaemon is the virtio-fs vhost-user daemon path VirtioFSDaemon string + // VirtioFSDaemonList is the list of valid virtiofs names for annotations + VirtioFSDaemonList []string + // VirtioFSCache cache mode for fs version cache or "none" VirtioFSCache string diff --git a/src/runtime/virtcontainers/persist.go b/src/runtime/virtcontainers/persist.go index 74f007d57c..94d3b0b08a 100644 --- a/src/runtime/virtcontainers/persist.go +++ b/src/runtime/virtcontainers/persist.go @@ -212,6 +212,7 @@ func (s *Sandbox) dumpConfig(ss *persistapi.SandboxState) { MachineAccelerators: sconfig.HypervisorConfig.MachineAccelerators, CPUFeatures: sconfig.HypervisorConfig.CPUFeatures, HypervisorPath: sconfig.HypervisorConfig.HypervisorPath, + HypervisorPathList: sconfig.HypervisorConfig.HypervisorPathList, HypervisorCtlPath: sconfig.HypervisorConfig.HypervisorCtlPath, JailerPath: sconfig.HypervisorConfig.JailerPath, BlockDeviceDriver: sconfig.HypervisorConfig.BlockDeviceDriver, @@ -221,6 +222,7 @@ func (s *Sandbox) dumpConfig(ss *persistapi.SandboxState) { EntropySource: sconfig.HypervisorConfig.EntropySource, SharedFS: sconfig.HypervisorConfig.SharedFS, VirtioFSDaemon: sconfig.HypervisorConfig.VirtioFSDaemon, + VirtioFSDaemonList: sconfig.HypervisorConfig.VirtioFSDaemonList, VirtioFSCache: sconfig.HypervisorConfig.VirtioFSCache, VirtioFSExtraArgs: sconfig.HypervisorConfig.VirtioFSExtraArgs[:], BlockDeviceCacheSet: sconfig.HypervisorConfig.BlockDeviceCacheSet, @@ -474,6 +476,7 @@ func loadSandboxConfig(id string) (*SandboxConfig, error) { MachineAccelerators: hconf.MachineAccelerators, CPUFeatures: hconf.CPUFeatures, HypervisorPath: hconf.HypervisorPath, + HypervisorPathList: hconf.HypervisorPathList, HypervisorCtlPath: hconf.HypervisorCtlPath, JailerPath: hconf.JailerPath, BlockDeviceDriver: hconf.BlockDeviceDriver, @@ -483,6 +486,7 @@ func loadSandboxConfig(id string) (*SandboxConfig, error) { EntropySource: hconf.EntropySource, SharedFS: hconf.SharedFS, VirtioFSDaemon: hconf.VirtioFSDaemon, + VirtioFSDaemonList: hconf.VirtioFSDaemonList, VirtioFSCache: hconf.VirtioFSCache, VirtioFSExtraArgs: hconf.VirtioFSExtraArgs[:], BlockDeviceCacheSet: hconf.BlockDeviceCacheSet, diff --git a/src/runtime/virtcontainers/persist/api/config.go b/src/runtime/virtcontainers/persist/api/config.go index 15092209b9..e3fef8ded5 100644 --- a/src/runtime/virtcontainers/persist/api/config.go +++ b/src/runtime/virtcontainers/persist/api/config.go @@ -60,6 +60,9 @@ type HypervisorConfig struct { // HypervisorPath is the hypervisor executable host path. HypervisorPath string + // HypervisorPathList is the list of hypervisor paths names allowed in annotations + HypervisorPathList []string + // HypervisorCtlPath is the hypervisor ctl executable host path. HypervisorCtlPath string @@ -94,6 +97,9 @@ type HypervisorConfig struct { // VirtioFSDaemon is the virtio-fs vhost-user daemon path VirtioFSDaemon string + // VirtioFSDaemonList is the list of valid virtiofs names for annotations + VirtioFSDaemonList []string + // VirtioFSCache cache mode for fs version cache or "none" VirtioFSCache string diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index 08baccb9a9..1ae45aecd1 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "path/filepath" + "regexp" goruntime "runtime" "strconv" "strings" @@ -191,6 +192,15 @@ func contains(s []string, e string) bool { return false } +func regexpContains(s []string, e string) bool { + for _, a := range s { + if matched, _ := regexp.MatchString(a, e); matched { + return true + } + } + return false +} + func newLinuxDeviceInfo(d specs.LinuxDevice) (*config.DeviceInfo, error) { allowedDeviceTypes := []string{"c", "b", "u", "p"} @@ -323,13 +333,13 @@ func SandboxID(spec specs.Spec) (string, error) { return "", fmt.Errorf("Could not find sandbox ID") } -func addAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) error { +func addAnnotations(ocispec specs.Spec, config *vc.SandboxConfig, runtime RuntimeConfig) error { addAssetAnnotations(ocispec, config) - if err := addHypervisorConfigOverrides(ocispec, config); err != nil { + if err := addHypervisorConfigOverrides(ocispec, config, runtime); err != nil { return err } - if err := addRuntimeConfigOverrides(ocispec, config); err != nil { + if err := addRuntimeConfigOverrides(ocispec, config, runtime); err != nil { return err } @@ -362,7 +372,7 @@ func addAssetAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) { } } -func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig) error { +func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, runtime RuntimeConfig) error { if err := addHypervisorCPUOverrides(ocispec, config); err != nil { return err } @@ -375,7 +385,7 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig) return err } - if err := addHypervisporVirtioFsOverrides(ocispec, config); err != nil { + if err := addHypervisporVirtioFsOverrides(ocispec, config, runtime); err != nil { return err } @@ -661,7 +671,7 @@ func addHypervisorBlockOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig) return nil } -func addHypervisporVirtioFsOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig) error { +func addHypervisporVirtioFsOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig, runtime RuntimeConfig) error { if value, ok := ocispec.Annotations[vcAnnotations.SharedFS]; ok { supportedSharedFS := []string{config.Virtio9P, config.VirtioFS} valid := false @@ -678,6 +688,9 @@ func addHypervisporVirtioFsOverrides(ocispec specs.Spec, sbConfig *vc.SandboxCon } if value, ok := ocispec.Annotations[vcAnnotations.VirtioFSDaemon]; ok { + if !regexpContains(runtime.HypervisorConfig.VirtioFSDaemonList, value) { + return fmt.Errorf("virtiofs daemon %v required from annotation is not valid", value) + } sbConfig.HypervisorConfig.VirtioFSDaemon = value } @@ -745,7 +758,7 @@ func addHypervisporNetworkOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConf return nil } -func addRuntimeConfigOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig) error { +func addRuntimeConfigOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig, runtime RuntimeConfig) error { if value, ok := ocispec.Annotations[vcAnnotations.DisableGuestSeccomp]; ok { disableGuestSeccomp, err := strconv.ParseBool(value) if err != nil { @@ -885,7 +898,7 @@ func SandboxConfig(ocispec specs.Spec, runtime RuntimeConfig, bundlePath, cid, c Experimental: runtime.Experimental, } - if err := addAnnotations(ocispec, &sandboxConfig); err != nil { + if err := addAnnotations(ocispec, &sandboxConfig, runtime); err != nil { return vc.SandboxConfig{}, err } diff --git a/src/runtime/virtcontainers/pkg/oci/utils_test.go b/src/runtime/virtcontainers/pkg/oci/utils_test.go index 828ecc56d9..6190203649 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils_test.go +++ b/src/runtime/virtcontainers/pkg/oci/utils_test.go @@ -676,7 +676,12 @@ func TestAddAssetAnnotations(t *testing.T) { Annotations: expectedAnnotations, } - addAnnotations(ocispec, &config) + runtimeConfig := RuntimeConfig{ + HypervisorType: vc.QemuHypervisor, + Console: consolePath, + } + + addAnnotations(ocispec, &config, runtimeConfig) assert.Exactly(expectedAnnotations, config.Annotations) } @@ -700,9 +705,14 @@ func TestAddAgentAnnotations(t *testing.T) { ContainerPipeSize: 1024, } + runtimeConfig := RuntimeConfig{ + HypervisorType: vc.QemuHypervisor, + Console: consolePath, + } + ocispec.Annotations[vcAnnotations.KernelModules] = strings.Join(expectedAgentConfig.KernelModules, KernelModulesSeparator) ocispec.Annotations[vcAnnotations.AgentContainerPipeSize] = "1024" - addAnnotations(ocispec, &config) + addAnnotations(ocispec, &config, runtimeConfig) assert.Exactly(expectedAgentConfig, config.AgentConfig) } @@ -722,8 +732,13 @@ func TestContainerPipeSizeAnnotation(t *testing.T) { ContainerPipeSize: 0, } + runtimeConfig := RuntimeConfig{ + HypervisorType: vc.QemuHypervisor, + Console: consolePath, + } + ocispec.Annotations[vcAnnotations.AgentContainerPipeSize] = "foo" - err := addAnnotations(ocispec, &config) + err := addAnnotations(ocispec, &config, runtimeConfig) assert.Error(err) assert.Exactly(expectedAgentConfig, config.AgentConfig) } @@ -752,8 +767,13 @@ func TestAddHypervisorAnnotations(t *testing.T) { }, } + runtimeConfig := RuntimeConfig{ + HypervisorType: vc.QemuHypervisor, + Console: consolePath, + } + ocispec.Annotations[vcAnnotations.KernelParams] = "vsyscall=emulate iommu=on" - addHypervisorConfigOverrides(ocispec, &config) + addHypervisorConfigOverrides(ocispec, &config, runtimeConfig) assert.Exactly(expectedHyperConfig, config.HypervisorConfig) ocispec.Annotations[vcAnnotations.DefaultVCPUs] = "1" @@ -792,7 +812,7 @@ func TestAddHypervisorAnnotations(t *testing.T) { ocispec.Annotations[vcAnnotations.RxRateLimiterMaxRate] = "10000000" ocispec.Annotations[vcAnnotations.TxRateLimiterMaxRate] = "10000000" - addAnnotations(ocispec, &config) + addAnnotations(ocispec, &config, runtimeConfig) assert.Equal(config.HypervisorConfig.NumVCPUs, uint32(1)) assert.Equal(config.HypervisorConfig.DefaultMaxVCPUs, uint32(1)) assert.Equal(config.HypervisorConfig.MemorySize, uint32(1024)) @@ -830,16 +850,16 @@ func TestAddHypervisorAnnotations(t *testing.T) { // In case an absurd large value is provided, the config value if not over-ridden ocispec.Annotations[vcAnnotations.DefaultVCPUs] = "655536" - err := addAnnotations(ocispec, &config) + err := addAnnotations(ocispec, &config, runtimeConfig) assert.Error(err) ocispec.Annotations[vcAnnotations.DefaultVCPUs] = "-1" - err = addAnnotations(ocispec, &config) + err = addAnnotations(ocispec, &config, runtimeConfig) assert.Error(err) ocispec.Annotations[vcAnnotations.DefaultVCPUs] = "1" ocispec.Annotations[vcAnnotations.DefaultMaxVCPUs] = "-1" - err = addAnnotations(ocispec, &config) + err = addAnnotations(ocispec, &config, runtimeConfig) assert.Error(err) ocispec.Annotations[vcAnnotations.DefaultMaxVCPUs] = "1" @@ -858,12 +878,17 @@ func TestAddRuntimeAnnotations(t *testing.T) { Annotations: make(map[string]string), } + runtimeConfig := RuntimeConfig{ + HypervisorType: vc.QemuHypervisor, + Console: consolePath, + } + ocispec.Annotations[vcAnnotations.DisableGuestSeccomp] = "true" ocispec.Annotations[vcAnnotations.SandboxCgroupOnly] = "true" ocispec.Annotations[vcAnnotations.DisableNewNetNs] = "true" ocispec.Annotations[vcAnnotations.InterNetworkModel] = "macvtap" - addAnnotations(ocispec, &config) + addAnnotations(ocispec, &config, runtimeConfig) assert.Equal(config.DisableGuestSeccomp, true) assert.Equal(config.SandboxCgroupOnly, true) assert.Equal(config.NetworkConfig.DisableNewNetNs, true) From 2e093dfd8ba55db3cee8fe8df47619a26c1faa1c Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 15 May 2020 15:59:30 +0200 Subject: [PATCH 03/24] config: Fix typo in function name There was an extra 'p' in addHypervisorVirtioFsOverrides. Fixes: #901 Signed-off-by: Christophe de Dinechin --- src/runtime/virtcontainers/pkg/oci/utils.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index 1ae45aecd1..ea84ced7f1 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -385,7 +385,7 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, return err } - if err := addHypervisporVirtioFsOverrides(ocispec, config, runtime); err != nil { + if err := addHypervisorVirtioFsOverrides(ocispec, config, runtime); err != nil { return err } @@ -671,7 +671,7 @@ func addHypervisorBlockOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig) return nil } -func addHypervisporVirtioFsOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig, runtime RuntimeConfig) error { +func addHypervisorVirtioFsOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig, runtime RuntimeConfig) error { if value, ok := ocispec.Annotations[vcAnnotations.SharedFS]; ok { supportedSharedFS := []string{config.Virtio9P, config.VirtioFS} valid := false From 2ca9ca892df8e01432b97d6c5d3e3eccf6d040d6 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 15 May 2020 16:04:55 +0200 Subject: [PATCH 04/24] config: Add hypervisor path override through annotations The annotation is provided, so it should be respected. Furthermore, it is important to implement it with the appropriate protetions similar to what was done for virtiofsd. Fixes: #901 Signed-off-by: Christophe de Dinechin --- src/runtime/virtcontainers/pkg/oci/utils.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index ea84ced7f1..f169e50fc1 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -393,6 +393,13 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, return err } + if value, ok := ocispec.Annotations[vcAnnotations.HypervisorPath]; ok { + if !regexpContains(runtime.HypervisorConfig.HypervisorPathList, value) { + return fmt.Errorf("hypervisor %v required from annotation is not valid", value) + } + config.HypervisorConfig.HypervisorPath = value + } + if value, ok := ocispec.Annotations[vcAnnotations.KernelParams]; ok { if value != "" { params := vc.DeserializeParams(strings.Fields(value)) From 2d431c61c69fd84b02793ff972157285d5f92687 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 15 May 2020 16:41:04 +0200 Subject: [PATCH 05/24] annotations: Simplify negative logic Replace strange negative logic (!ok -> continue) with positive logic (ok -> do it) Fixes: #901 Signed-off-by: Christophe de Dinechin --- src/runtime/virtcontainers/pkg/oci/utils.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index f169e50fc1..fc0d291c96 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -364,11 +364,9 @@ func addAssetAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) { for _, a := range assetAnnotations { value, ok := ocispec.Annotations[a] - if !ok { - continue + if ok { + config.Annotations[a] = value } - - config.Annotations[a] = value } } From 076690179d7c55dceaec23fbfb38c87b732e8f6e Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 15 May 2020 17:40:43 +0200 Subject: [PATCH 06/24] config: Add examples for path_list configuration The path_list configuration gives a series of regular expressions that limit which values are acceptable through annotations in order to avoid kata launching arbitrary binaries on the host when receiving an annotation. Fixes: #901 Signed-off-by: Christophe de Dinechin --- .../cli/config/configuration-acrn.toml.in | 4 ++++ .../cli/config/configuration-clh.toml.in | 3 +++ .../cli/config/configuration-fc.toml.in | 18 ++++++++++++------ .../config/configuration-qemu-virtiofs.toml.in | 4 ++++ 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/runtime/cli/config/configuration-acrn.toml.in b/src/runtime/cli/config/configuration-acrn.toml.in index 15fb00e45d..7bc8d3172e 100644 --- a/src/runtime/cli/config/configuration-acrn.toml.in +++ b/src/runtime/cli/config/configuration-acrn.toml.in @@ -16,6 +16,10 @@ ctlpath = "@ACRNCTLPATH@" kernel = "@KERNELPATH_ACRN@" image = "@IMAGEPATH@" +# List of valid annotations values for the hypervisor (default: empty) +# Each member of the list can be a regular expression +# path_list = [ "@ACRNPATH@.*" ] + # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having # trouble running pre-2.15 glibc. diff --git a/src/runtime/cli/config/configuration-clh.toml.in b/src/runtime/cli/config/configuration-clh.toml.in index b3a74705ac..2b9f1b6b58 100644 --- a/src/runtime/cli/config/configuration-clh.toml.in +++ b/src/runtime/cli/config/configuration-clh.toml.in @@ -12,6 +12,9 @@ [hypervisor.clh] path = "@CLHPATH@" +# List of valid annotations values for the hypervisor (default: empty) +# Each member of the list can be a regular expression +# path_list = [ "@CLHPATH@.*" ] kernel = "@KERNELPATH_CLH@" image = "@IMAGEPATH@" diff --git a/src/runtime/cli/config/configuration-fc.toml.in b/src/runtime/cli/config/configuration-fc.toml.in index e72130e0ed..7aa638e849 100644 --- a/src/runtime/cli/config/configuration-fc.toml.in +++ b/src/runtime/cli/config/configuration-fc.toml.in @@ -12,6 +12,13 @@ [hypervisor.firecracker] path = "@FCPATH@" +kernel = "@KERNELPATH_FC@" +image = "@IMAGEPATH@" + +# List of valid annotations values for the hypervisor (default: empty) +# Each member of the list can be a regular expression +# path_list = [ "@FCPATH@.*" ] + # Path for the jailer specific to firecracker # If the jailer path is not set kata will launch firecracker # without a jail. If the jailer is set firecracker will be @@ -19,8 +26,7 @@ path = "@FCPATH@" # This is disabled by default as additional setup is required # for this feature today. #jailer_path = "@FCJAILERPATH@" -kernel = "@KERNELPATH_FC@" -image = "@IMAGEPATH@" + # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having @@ -87,10 +93,10 @@ default_memory = @DEFMEMSZ@ #memory_offset = 0 # Disable block device from being used for a container's rootfs. -# In case of a storage driver like devicemapper where a container's +# In case of a storage driver like devicemapper where a container's # root file system is backed by a block device, the block device is passed -# directly to the hypervisor for performance reasons. -# This flag prevents the block device from being passed to the hypervisor, +# directly to the hypervisor for performance reasons. +# This flag prevents the block device from being passed to the hypervisor, # 9pfs is used instead to pass the rootfs. disable_block_device_use = @DEFDISABLEBLOCK@ @@ -126,7 +132,7 @@ block_device_driver = "@DEFBLOCKSTORAGEDRIVER_FC@" # Enabling this will result in the VM memory # being allocated using huge pages. # This is useful when you want to use vhost-user network -# stacks within the container. This will automatically +# stacks within the container. This will automatically # result in memory pre allocation #enable_hugepages = true diff --git a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in index 4a1523b439..cc0c63e483 100644 --- a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in +++ b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in @@ -16,6 +16,10 @@ kernel = "@KERNELVIRTIOFSPATH@" image = "@IMAGEPATH@" machine_type = "@MACHINETYPE@" +# List of valid annotations values for the hypervisor (default: empty) +# Each member of the list can be a regular expression +# path_list = [ "@QEMUPATH@.*" ] + # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having # trouble running pre-2.15 glibc. From 27b6620b2311bee3d0b88dc85176b387133b330f Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 15 May 2020 17:49:01 +0200 Subject: [PATCH 07/24] config: Protect jailer_path annotation The jailer_path annotation can be used to execute arbitrary code on the host. Add a jailer_path_list configuration entry providing a list of regular expressions that can be used to filter annotations that represent valid file names. Fixes: #901 Signed-off-by: Christophe de Dinechin --- src/runtime/cli/config/configuration-fc.toml.in | 4 ++++ src/runtime/pkg/katautils/config.go | 1 + src/runtime/virtcontainers/hypervisor.go | 3 +++ src/runtime/virtcontainers/persist.go | 2 ++ src/runtime/virtcontainers/persist/api/config.go | 3 +++ src/runtime/virtcontainers/pkg/oci/utils.go | 7 +++++++ 6 files changed, 20 insertions(+) diff --git a/src/runtime/cli/config/configuration-fc.toml.in b/src/runtime/cli/config/configuration-fc.toml.in index 7aa638e849..f54b7ba4ae 100644 --- a/src/runtime/cli/config/configuration-fc.toml.in +++ b/src/runtime/cli/config/configuration-fc.toml.in @@ -27,6 +27,10 @@ image = "@IMAGEPATH@" # for this feature today. #jailer_path = "@FCJAILERPATH@" +# List of valid jailer path values for the hypervisor (default: empty) +# Each member of the list can be a regular expression +# jailer_path_list = [ "@FCJAILERPATH@.*" ] + # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having diff --git a/src/runtime/pkg/katautils/config.go b/src/runtime/pkg/katautils/config.go index 6bf59090e8..a71d4064e1 100644 --- a/src/runtime/pkg/katautils/config.go +++ b/src/runtime/pkg/katautils/config.go @@ -535,6 +535,7 @@ func newFirecrackerHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { HypervisorPath: hypervisor, HypervisorPathList: h.HypervisorPathList, JailerPath: jailer, + JailerPathList: h.JailerPathList, KernelPath: kernel, InitrdPath: initrd, ImagePath: image, diff --git a/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index 525133d59a..b5a7122258 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/virtcontainers/hypervisor.go @@ -284,6 +284,9 @@ type HypervisorConfig struct { // JailerPath is the jailer executable host path. JailerPath string + // JailerPathList is the list of jailer paths names allowed in annotations + JailerPathList []string + // BlockDeviceDriver specifies the driver to be used for block device // either VirtioSCSI or VirtioBlock with the default driver being defaultBlockDriver BlockDeviceDriver string diff --git a/src/runtime/virtcontainers/persist.go b/src/runtime/virtcontainers/persist.go index 94d3b0b08a..2b8a0ee436 100644 --- a/src/runtime/virtcontainers/persist.go +++ b/src/runtime/virtcontainers/persist.go @@ -215,6 +215,7 @@ func (s *Sandbox) dumpConfig(ss *persistapi.SandboxState) { HypervisorPathList: sconfig.HypervisorConfig.HypervisorPathList, HypervisorCtlPath: sconfig.HypervisorConfig.HypervisorCtlPath, JailerPath: sconfig.HypervisorConfig.JailerPath, + JailerPathList: sconfig.HypervisorConfig.JailerPathList, BlockDeviceDriver: sconfig.HypervisorConfig.BlockDeviceDriver, HypervisorMachineType: sconfig.HypervisorConfig.HypervisorMachineType, MemoryPath: sconfig.HypervisorConfig.MemoryPath, @@ -479,6 +480,7 @@ func loadSandboxConfig(id string) (*SandboxConfig, error) { HypervisorPathList: hconf.HypervisorPathList, HypervisorCtlPath: hconf.HypervisorCtlPath, JailerPath: hconf.JailerPath, + JailerPathList: hconf.JailerPathList, BlockDeviceDriver: hconf.BlockDeviceDriver, HypervisorMachineType: hconf.HypervisorMachineType, MemoryPath: hconf.MemoryPath, diff --git a/src/runtime/virtcontainers/persist/api/config.go b/src/runtime/virtcontainers/persist/api/config.go index e3fef8ded5..4dc7849b7c 100644 --- a/src/runtime/virtcontainers/persist/api/config.go +++ b/src/runtime/virtcontainers/persist/api/config.go @@ -69,6 +69,9 @@ type HypervisorConfig struct { // JailerPath is the jailer executable host path. JailerPath string + // JailerPathList is the list of jailer paths names allowed in annotations + JailerPathList []string + // BlockDeviceDriver specifies the driver to be used for block device // either VirtioSCSI or VirtioBlock with the default driver being defaultBlockDriver BlockDeviceDriver string diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index fc0d291c96..5b4edb35b6 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -398,6 +398,13 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, config.HypervisorConfig.HypervisorPath = value } + if value, ok := ocispec.Annotations[vcAnnotations.JailerPath]; ok { + if !regexpContains(runtime.HypervisorConfig.JailerPathList, value) { + return fmt.Errorf("jailer %v required from annotation is not valid", value) + } + config.HypervisorConfig.JailerPath = value + } + if value, ok := ocispec.Annotations[vcAnnotations.KernelParams]; ok { if value != "" { params := vc.DeserializeParams(strings.Fields(value)) From b21a829c61a687555c12b82691cb5267fd671f8c Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 15 May 2020 18:10:53 +0200 Subject: [PATCH 08/24] config: Protect ctlpath from annotation attack This also adds annotation for ctlpath which were not present before. It's better to implement the code consistenly right now to make sure that we don't end up with a leaky implementation tacked on later. Fixes: #901 Signed-off-by: Christophe de Dinechin --- .../cli/config/configuration-acrn.toml.in | 3 ++ src/runtime/pkg/katautils/config.go | 41 ++++++++++--------- src/runtime/virtcontainers/hypervisor.go | 3 ++ src/runtime/virtcontainers/persist.go | 2 + .../virtcontainers/persist/api/config.go | 4 ++ .../pkg/annotations/annotations.go | 3 ++ src/runtime/virtcontainers/pkg/oci/utils.go | 7 ++++ 7 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/runtime/cli/config/configuration-acrn.toml.in b/src/runtime/cli/config/configuration-acrn.toml.in index 7bc8d3172e..f6e9b7849e 100644 --- a/src/runtime/cli/config/configuration-acrn.toml.in +++ b/src/runtime/cli/config/configuration-acrn.toml.in @@ -20,6 +20,9 @@ image = "@IMAGEPATH@" # Each member of the list can be a regular expression # path_list = [ "@ACRNPATH@.*" ] +# List of valid annotations values for ctlpath (default: empty) +# ctlpath_list = [ "@ACRNCTLPATH@.*" ] + # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having # trouble running pre-2.15 glibc. diff --git a/src/runtime/pkg/katautils/config.go b/src/runtime/pkg/katautils/config.go index a71d4064e1..5b739a682d 100644 --- a/src/runtime/pkg/katautils/config.go +++ b/src/runtime/pkg/katautils/config.go @@ -725,26 +725,27 @@ func newAcrnHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { } return vc.HypervisorConfig{ - HypervisorPath: hypervisor, - HypervisorPathList: h.HypervisorPathList, - KernelPath: kernel, - ImagePath: image, - HypervisorCtlPath: hypervisorctl, - FirmwarePath: firmware, - KernelParams: vc.DeserializeParams(strings.Fields(kernelParams)), - NumVCPUs: h.defaultVCPUs(), - DefaultMaxVCPUs: h.defaultMaxVCPUs(), - MemorySize: h.defaultMemSz(), - MemSlots: h.defaultMemSlots(), - EntropySource: h.GetEntropySource(), - DefaultBridges: h.defaultBridges(), - HugePages: h.HugePages, - Mlock: !h.Swap, - Debug: h.Debug, - DisableNestingChecks: h.DisableNestingChecks, - BlockDeviceDriver: blockDriver, - DisableVhostNet: h.DisableVhostNet, - GuestHookPath: h.guestHookPath(), + HypervisorPath: hypervisor, + HypervisorPathList: h.HypervisorPathList, + KernelPath: kernel, + ImagePath: image, + HypervisorCtlPath: hypervisorctl, + HypervisorCtlPathList: h.CtlPathList, + FirmwarePath: firmware, + KernelParams: vc.DeserializeParams(strings.Fields(kernelParams)), + NumVCPUs: h.defaultVCPUs(), + DefaultMaxVCPUs: h.defaultMaxVCPUs(), + MemorySize: h.defaultMemSz(), + MemSlots: h.defaultMemSlots(), + EntropySource: h.GetEntropySource(), + DefaultBridges: h.defaultBridges(), + HugePages: h.HugePages, + Mlock: !h.Swap, + Debug: h.Debug, + DisableNestingChecks: h.DisableNestingChecks, + BlockDeviceDriver: blockDriver, + DisableVhostNet: h.DisableVhostNet, + GuestHookPath: h.guestHookPath(), }, nil } diff --git a/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index b5a7122258..e006ed7582 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/virtcontainers/hypervisor.go @@ -278,6 +278,9 @@ type HypervisorConfig struct { // HypervisorPathList is the list of hypervisor paths names allowed in annotations HypervisorPathList []string + // HypervisorCtlPathList is the list of hypervisor control paths names allowed in annotations + HypervisorCtlPathList []string + // HypervisorCtlPath is the hypervisor ctl executable host path. HypervisorCtlPath string diff --git a/src/runtime/virtcontainers/persist.go b/src/runtime/virtcontainers/persist.go index 2b8a0ee436..7630bc2f83 100644 --- a/src/runtime/virtcontainers/persist.go +++ b/src/runtime/virtcontainers/persist.go @@ -214,6 +214,7 @@ func (s *Sandbox) dumpConfig(ss *persistapi.SandboxState) { HypervisorPath: sconfig.HypervisorConfig.HypervisorPath, HypervisorPathList: sconfig.HypervisorConfig.HypervisorPathList, HypervisorCtlPath: sconfig.HypervisorConfig.HypervisorCtlPath, + HypervisorCtlPathList: sconfig.HypervisorConfig.HypervisorCtlPathList, JailerPath: sconfig.HypervisorConfig.JailerPath, JailerPathList: sconfig.HypervisorConfig.JailerPathList, BlockDeviceDriver: sconfig.HypervisorConfig.BlockDeviceDriver, @@ -479,6 +480,7 @@ func loadSandboxConfig(id string) (*SandboxConfig, error) { HypervisorPath: hconf.HypervisorPath, HypervisorPathList: hconf.HypervisorPathList, HypervisorCtlPath: hconf.HypervisorCtlPath, + HypervisorCtlPathList: hconf.HypervisorCtlPathList, JailerPath: hconf.JailerPath, JailerPathList: hconf.JailerPathList, BlockDeviceDriver: hconf.BlockDeviceDriver, diff --git a/src/runtime/virtcontainers/persist/api/config.go b/src/runtime/virtcontainers/persist/api/config.go index 4dc7849b7c..a4e815f94f 100644 --- a/src/runtime/virtcontainers/persist/api/config.go +++ b/src/runtime/virtcontainers/persist/api/config.go @@ -66,6 +66,10 @@ type HypervisorConfig struct { // HypervisorCtlPath is the hypervisor ctl executable host path. HypervisorCtlPath string + // HypervisorCtlPathList is the list of hypervisor control paths names allowed in annotations + HypervisorCtlPathList []string + + // HypervisorCtlPath is the hypervisor ctl executable host path. // JailerPath is the jailer executable host path. JailerPath string diff --git a/src/runtime/virtcontainers/pkg/annotations/annotations.go b/src/runtime/virtcontainers/pkg/annotations/annotations.go index ff9bd13e47..5e8f52c503 100644 --- a/src/runtime/virtcontainers/pkg/annotations/annotations.go +++ b/src/runtime/virtcontainers/pkg/annotations/annotations.go @@ -44,6 +44,9 @@ const ( // JailerPath is a sandbox annotation for passing a per container path pointing at the jailer that will constrain the container VM. JailerPath = kataAnnotHypervisorPrefix + "jailer_path" + // CtlPath is a sandbox annotation for passing a per container path pointing at the acrn ctl binary + CtlPath = kataAnnotHypervisorPrefix + "ctlpath" + // FirmwarePath is a sandbox annotation for passing a per container path pointing at the guest firmware that will run the container VM. FirmwarePath = kataAnnotHypervisorPrefix + "firmware" diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index 5b4edb35b6..c3b66433f1 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -405,6 +405,13 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, config.HypervisorConfig.JailerPath = value } + if value, ok := ocispec.Annotations[vcAnnotations.CtlPath]; ok { + if !regexpContains(runtime.HypervisorConfig.HypervisorCtlPathList, value) { + return fmt.Errorf("hypervisor control %v required from annotation is not valid", value) + } + config.HypervisorConfig.HypervisorCtlPath = value + } + if value, ok := ocispec.Annotations[vcAnnotations.KernelParams]; ok { if value != "" { params := vc.DeserializeParams(strings.Fields(value)) From 55881653996f7c763e8c741ba4c8afdfd2c2bfd6 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 15 May 2020 18:26:29 +0200 Subject: [PATCH 09/24] config: Add security warning on configuration examples Add the following text explaining the risk of using regular expressions in path lists: Each member of the list can be a regular expression, but prefer names. Otherwise, please read and understand the following carefully. SECURITY WARNING: If you use regular expressions, be mindful that an attacker could craft an annotation that uses .. to escape the paths you gave. For example, if your regexp is /bin/qemu.* then if there is a directory named /bin/qemu.d/, then an attacker can pass an annotation containing /bin/qemu.d/../put-any-binary-name-here and attack your host. Fixes: #901 Signed-off-by: Christophe de Dinechin --- src/runtime/cli/config/configuration-acrn.toml.in | 8 +++++++- src/runtime/cli/config/configuration-clh.toml.in | 13 ++++++++++--- src/runtime/cli/config/configuration-fc.toml.in | 8 +++++++- .../config/configuration-qemu-virtiofs.toml.in | 8 +++++++- src/runtime/cli/config/configuration-qemu.toml.in | 15 ++++++++++++--- 5 files changed, 43 insertions(+), 9 deletions(-) diff --git a/src/runtime/cli/config/configuration-acrn.toml.in b/src/runtime/cli/config/configuration-acrn.toml.in index f6e9b7849e..d56b2ec9bb 100644 --- a/src/runtime/cli/config/configuration-acrn.toml.in +++ b/src/runtime/cli/config/configuration-acrn.toml.in @@ -17,7 +17,13 @@ kernel = "@KERNELPATH_ACRN@" image = "@IMAGEPATH@" # List of valid annotations values for the hypervisor (default: empty) -# Each member of the list can be a regular expression +# Each member of the list can be a regular expression, but prefer names. +# Otherwise, please read and understand the following carefully. +# SECURITY WARNING: If you use regular expressions, be mindful that +# an attacker could craft an annotation that uses .. to escape the paths +# you gave. For example, if your regexp is /bin/qemu.* then if there is +# a directory named /bin/qemu.d/, then an attacker can pass an annotation +# containing /bin/qemu.d/../put-any-binary-name-here and attack your host. # path_list = [ "@ACRNPATH@.*" ] # List of valid annotations values for ctlpath (default: empty) diff --git a/src/runtime/cli/config/configuration-clh.toml.in b/src/runtime/cli/config/configuration-clh.toml.in index 2b9f1b6b58..8e2419f62e 100644 --- a/src/runtime/cli/config/configuration-clh.toml.in +++ b/src/runtime/cli/config/configuration-clh.toml.in @@ -12,12 +12,19 @@ [hypervisor.clh] path = "@CLHPATH@" -# List of valid annotations values for the hypervisor (default: empty) -# Each member of the list can be a regular expression -# path_list = [ "@CLHPATH@.*" ] kernel = "@KERNELPATH_CLH@" image = "@IMAGEPATH@" +# List of valid annotations values for the hypervisor (default: empty) +# Each member of the list can be a regular expression, but prefer names. +# Otherwise, please read and understand the following carefully. +# SECURITY WARNING: If you use regular expressions, be mindful that +# an attacker could craft an annotation that uses .. to escape the paths +# you gave. For example, if your regexp is /bin/qemu.* then if there is +# a directory named /bin/qemu.d/, then an attacker can pass an annotation +# containing /bin/qemu.d/../put-any-binary-name-here and attack your host. +# path_list = [ "@CLHPATH@.*" ] + # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having # trouble running pre-2.15 glibc. diff --git a/src/runtime/cli/config/configuration-fc.toml.in b/src/runtime/cli/config/configuration-fc.toml.in index f54b7ba4ae..2c239cd4e7 100644 --- a/src/runtime/cli/config/configuration-fc.toml.in +++ b/src/runtime/cli/config/configuration-fc.toml.in @@ -16,7 +16,13 @@ kernel = "@KERNELPATH_FC@" image = "@IMAGEPATH@" # List of valid annotations values for the hypervisor (default: empty) -# Each member of the list can be a regular expression +# Each member of the list can be a regular expression, but prefer names. +# Otherwise, please read and understand the following carefully. +# SECURITY WARNING: If you use regular expressions, be mindful that +# an attacker could craft an annotation that uses .. to escape the paths +# you gave. For example, if your regexp is /bin/qemu.* then if there is +# a directory named /bin/qemu.d/, then an attacker can pass an annotation +# containing /bin/qemu.d/../put-any-binary-name-here and attack your host. # path_list = [ "@FCPATH@.*" ] # Path for the jailer specific to firecracker diff --git a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in index cc0c63e483..ee27f6e62a 100644 --- a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in +++ b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in @@ -17,7 +17,13 @@ image = "@IMAGEPATH@" machine_type = "@MACHINETYPE@" # List of valid annotations values for the hypervisor (default: empty) -# Each member of the list can be a regular expression +# Each member of the list can be a regular expression, but prefer names. +# Otherwise, please read and understand the following carefully. +# SECURITY WARNING: If you use regular expressions, be mindful that +# an attacker could craft an annotation that uses .. to escape the paths +# you gave. For example, if your regexp is /bin/qemu.* then if there is +# a directory named /bin/qemu.d/, then an attacker can pass an annotation +# containing /bin/qemu.d/../put-any-binary-name-here and attack your host. # path_list = [ "@QEMUPATH@.*" ] # Optional space-separated list of options to pass to the guest kernel. diff --git a/src/runtime/cli/config/configuration-qemu.toml.in b/src/runtime/cli/config/configuration-qemu.toml.in index 22054c0374..21c2a2b426 100644 --- a/src/runtime/cli/config/configuration-qemu.toml.in +++ b/src/runtime/cli/config/configuration-qemu.toml.in @@ -12,6 +12,15 @@ [hypervisor.qemu] path = "@QEMUPATH@" +# List of valid annotations values for the hypervisor (default: empty) +# Each member of the list can be a regular expression, but prefer names. +# Otherwise, please read and understand the following carefully. +# SECURITY WARNING: If you use regular expressions, be mindful that +# an attacker could craft an annotation that uses .. to escape the paths +# you gave. For example, if your regexp is /bin/qemu.* then if there is +# a directory named /bin/qemu.d/, then an attacker can pass an annotation +# containing /bin/qemu.d/../put-any-binary-name-here and attack your host. +# path_list = [ "@QEMUPATH@.*" ] kernel = "@KERNELPATH@" image = "@IMAGEPATH@" machine_type = "@MACHINETYPE@" @@ -220,17 +229,17 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # This option changes the default hypervisor and kernel parameters # to enable debug output where available. -# +# # Default false #enable_debug = true # Disable the customizations done in the runtime when it detects # that it is running on top a VMM. This will result in the runtime # behaving as it would when running on bare metal. -# +# #disable_nesting_checks = true -# This is the msize used for 9p shares. It is the number of bytes +# This is the msize used for 9p shares. It is the number of bytes # used for 9p packet payload. #msize_9p = @DEFMSIZE9P@ From aae9656d8ba146611be46a128281d68449cfc1a9 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 15 May 2020 18:42:30 +0200 Subject: [PATCH 10/24] config: Protect vhost_user_store_path against annotation attacks This path could be used to overwrite data on the host. Fixes: #901 Signed-off-by: Christophe de Dinechin --- src/runtime/cli/config/configuration-qemu-virtiofs.toml.in | 3 +++ src/runtime/cli/config/configuration-qemu.toml.in | 3 +++ src/runtime/pkg/katautils/config.go | 1 + src/runtime/virtcontainers/hypervisor.go | 3 +++ src/runtime/virtcontainers/persist.go | 2 ++ src/runtime/virtcontainers/persist/api/config.go | 3 +++ src/runtime/virtcontainers/pkg/oci/utils.go | 7 +++++++ 7 files changed, 22 insertions(+) diff --git a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in index ee27f6e62a..6ec254fe55 100644 --- a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in +++ b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in @@ -212,6 +212,9 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # Enabling this will result in the VM device having iommu_platform=on set #enable_iommu_platform = true +# List of valid annotations values for the virtiofs daemon (default: empty) +# vhost_user_store_path_list = [ "/empty/space", "/multiverse/quantum-foam" ] + # Enable file based guest memory support. The default is an empty string which # will disable this feature. In the case of virtio-fs, this is enabled # automatically and '/dev/shm' is used as the backing folder. diff --git a/src/runtime/cli/config/configuration-qemu.toml.in b/src/runtime/cli/config/configuration-qemu.toml.in index 21c2a2b426..d1f63f7e7f 100644 --- a/src/runtime/cli/config/configuration-qemu.toml.in +++ b/src/runtime/cli/config/configuration-qemu.toml.in @@ -217,6 +217,9 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # Enabling this will result in the VM device having iommu_platform=on set #enable_iommu_platform = true +# List of valid annotations values for the virtiofs daemon (default: empty) +# vhost_user_store_path_list = [ "/empty/space", "/multiverse/quantum-foam" ] + # Enable file based guest memory support. The default is an empty string which # will disable this feature. In the case of virtio-fs, this is enabled # automatically and '/dev/shm' is used as the backing folder. diff --git a/src/runtime/pkg/katautils/config.go b/src/runtime/pkg/katautils/config.go index 5b739a682d..1ce463417f 100644 --- a/src/runtime/pkg/katautils/config.go +++ b/src/runtime/pkg/katautils/config.go @@ -680,6 +680,7 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { DisableVhostNet: h.DisableVhostNet, EnableVhostUserStore: h.EnableVhostUserStore, VhostUserStorePath: h.vhostUserStorePath(), + VhostUserStorePathList: h.VhostUserStorePathList, GuestHookPath: h.guestHookPath(), RxRateLimiterMaxRate: rxRateLimiterMaxRate, TxRateLimiterMaxRate: txRateLimiterMaxRate, diff --git a/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index e006ed7582..2f43f699df 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/virtcontainers/hypervisor.go @@ -412,6 +412,9 @@ type HypervisorConfig struct { // related folders, sockets and device nodes should be. VhostUserStorePath string + // VhostUserStorePathList is the list of valid values for vhost-user paths + VhostUserStorePathList []string + // GuestHookPath is the path within the VM that will be used for 'drop-in' hooks GuestHookPath string diff --git a/src/runtime/virtcontainers/persist.go b/src/runtime/virtcontainers/persist.go index 7630bc2f83..cdb90cb8d0 100644 --- a/src/runtime/virtcontainers/persist.go +++ b/src/runtime/virtcontainers/persist.go @@ -247,6 +247,7 @@ func (s *Sandbox) dumpConfig(ss *persistapi.SandboxState) { DisableVhostNet: sconfig.HypervisorConfig.DisableVhostNet, EnableVhostUserStore: sconfig.HypervisorConfig.EnableVhostUserStore, VhostUserStorePath: sconfig.HypervisorConfig.VhostUserStorePath, + VhostUserStorePathList: sconfig.HypervisorConfig.VhostUserStorePathList, GuestHookPath: sconfig.HypervisorConfig.GuestHookPath, VMid: sconfig.HypervisorConfig.VMid, RxRateLimiterMaxRate: sconfig.HypervisorConfig.RxRateLimiterMaxRate, @@ -513,6 +514,7 @@ func loadSandboxConfig(id string) (*SandboxConfig, error) { DisableVhostNet: hconf.DisableVhostNet, EnableVhostUserStore: hconf.EnableVhostUserStore, VhostUserStorePath: hconf.VhostUserStorePath, + VhostUserStorePathList: hconf.VhostUserStorePathList, GuestHookPath: hconf.GuestHookPath, VMid: hconf.VMid, RxRateLimiterMaxRate: hconf.RxRateLimiterMaxRate, diff --git a/src/runtime/virtcontainers/persist/api/config.go b/src/runtime/virtcontainers/persist/api/config.go index a4e815f94f..10fee16395 100644 --- a/src/runtime/virtcontainers/persist/api/config.go +++ b/src/runtime/virtcontainers/persist/api/config.go @@ -186,6 +186,9 @@ type HypervisorConfig struct { // related folders, sockets and device nodes should be. VhostUserStorePath string + // VhostUserStorePathList is the list of valid values for vhost-user paths + VhostUserStorePathList []string + // GuestHookPath is the path within the VM that will be used for 'drop-in' hooks GuestHookPath string diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index c3b66433f1..6a14c39dd3 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -435,6 +435,13 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, } } + if value, ok := ocispec.Annotations[vcAnnotations.VhostUserStorePath]; ok { + if !regexpContains(runtime.HypervisorConfig.VhostUserStorePathList, value) { + return fmt.Errorf("vhost store path %v required from annotation is not valid", value) + } + config.HypervisorConfig.VhostUserStorePath = value + } + if value, ok := ocispec.Annotations[vcAnnotations.GuestHookPath]; ok { if value != "" { config.HypervisorConfig.GuestHookPath = value From 4e89b885d25e5701c068b1c9e76d35fd05e20919 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 15 May 2020 18:55:02 +0200 Subject: [PATCH 11/24] config: Protect file_mem_backend against annotation attacks This one could theoretically be used to overwrite data on the host. It seems somewhat less risky than the earlier ones for a number of reasons, but worth protecting a little anyway. Fixes: #901 Signed-off-by: Christophe de Dinechin --- src/runtime/cli/config/configuration-qemu-virtiofs.toml.in | 3 +++ src/runtime/cli/config/configuration-qemu.toml.in | 3 +++ src/runtime/pkg/katautils/config.go | 2 ++ src/runtime/virtcontainers/hypervisor.go | 3 +++ src/runtime/virtcontainers/persist.go | 2 ++ src/runtime/virtcontainers/persist/api/config.go | 3 +++ src/runtime/virtcontainers/pkg/oci/utils.go | 7 +++++-- 7 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in index 6ec254fe55..d9564adfb3 100644 --- a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in +++ b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in @@ -221,6 +221,9 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # This option will be ignored if VM templating is enabled. #file_mem_backend = "" +# List of valid annotations values for the file_mem_backend annotation (default: empty) +# file_mem_backend_list = [ "/dev/shm" ] + # Enable swap of vm memory. Default false. # The behaviour is undefined if mem_prealloc is also set to true #enable_swap = true diff --git a/src/runtime/cli/config/configuration-qemu.toml.in b/src/runtime/cli/config/configuration-qemu.toml.in index d1f63f7e7f..e87daa107e 100644 --- a/src/runtime/cli/config/configuration-qemu.toml.in +++ b/src/runtime/cli/config/configuration-qemu.toml.in @@ -226,6 +226,9 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # This option will be ignored if VM templating is enabled. #file_mem_backend = "" +# List of valid annotations values for the file_mem_backend annotation (default: empty) +# file_mem_backend_list = [ "/dev/shm" ] + # Enable swap of vm memory. Default false. # The behaviour is undefined if mem_prealloc is also set to true #enable_swap = true diff --git a/src/runtime/pkg/katautils/config.go b/src/runtime/pkg/katautils/config.go index 1ce463417f..d266bd55fd 100644 --- a/src/runtime/pkg/katautils/config.go +++ b/src/runtime/pkg/katautils/config.go @@ -665,6 +665,7 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { IOMMU: h.IOMMU, IOMMUPlatform: h.getIOMMUPlatform(), FileBackedMemRootDir: h.FileBackedMemRootDir, + FileBackedMemRootList: h.FileBackedMemRootList, Mlock: !h.Swap, Debug: h.Debug, DisableNestingChecks: h.DisableNestingChecks, @@ -824,6 +825,7 @@ func newClhHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { MemPrealloc: h.MemPrealloc, HugePages: h.HugePages, FileBackedMemRootDir: h.FileBackedMemRootDir, + FileBackedMemRootList: h.FileBackedMemRootList, Mlock: !h.Swap, Debug: h.Debug, DisableNestingChecks: h.DisableNestingChecks, diff --git a/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index 2f43f699df..46fac8907d 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/virtcontainers/hypervisor.go @@ -330,6 +330,9 @@ type HypervisorConfig struct { // File based memory backend root directory FileBackedMemRootDir string + // FileBackedMemRootList is the list of valid root directories values for annotations + FileBackedMemRootList []string + // customAssets is a map of assets. // Each value in that map takes precedence over the configured assets. // For example, if there is a value for the "kernel" key in this map, diff --git a/src/runtime/virtcontainers/persist.go b/src/runtime/virtcontainers/persist.go index cdb90cb8d0..65399e86af 100644 --- a/src/runtime/virtcontainers/persist.go +++ b/src/runtime/virtcontainers/persist.go @@ -236,6 +236,7 @@ func (s *Sandbox) dumpConfig(ss *persistapi.SandboxState) { MemPrealloc: sconfig.HypervisorConfig.MemPrealloc, HugePages: sconfig.HypervisorConfig.HugePages, FileBackedMemRootDir: sconfig.HypervisorConfig.FileBackedMemRootDir, + FileBackedMemRootList: sconfig.HypervisorConfig.FileBackedMemRootList, Realtime: sconfig.HypervisorConfig.Realtime, Mlock: sconfig.HypervisorConfig.Mlock, DisableNestingChecks: sconfig.HypervisorConfig.DisableNestingChecks, @@ -503,6 +504,7 @@ func loadSandboxConfig(id string) (*SandboxConfig, error) { MemPrealloc: hconf.MemPrealloc, HugePages: hconf.HugePages, FileBackedMemRootDir: hconf.FileBackedMemRootDir, + FileBackedMemRootList: hconf.FileBackedMemRootList, Realtime: hconf.Realtime, Mlock: hconf.Mlock, DisableNestingChecks: hconf.DisableNestingChecks, diff --git a/src/runtime/virtcontainers/persist/api/config.go b/src/runtime/virtcontainers/persist/api/config.go index 10fee16395..025c93d1a9 100644 --- a/src/runtime/virtcontainers/persist/api/config.go +++ b/src/runtime/virtcontainers/persist/api/config.go @@ -116,6 +116,9 @@ type HypervisorConfig struct { // File based memory backend root directory FileBackedMemRootDir string + // FileBackedMemRootList is the list of valid root directories values for annotations + FileBackedMemRootList []string + // BlockDeviceCacheSet specifies cache-related options will be set to block devices or not. BlockDeviceCacheSet bool diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index 6a14c39dd3..2053d4d403 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -375,7 +375,7 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, return err } - if err := addHypervisorMemoryOverrides(ocispec, config); err != nil { + if err := addHypervisorMemoryOverrides(ocispec, config, runtime); err != nil { return err } @@ -497,7 +497,7 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, return nil } -func addHypervisorMemoryOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig) error { +func addHypervisorMemoryOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig, runtime RuntimeConfig) error { if value, ok := ocispec.Annotations[vcAnnotations.DefaultMemory]; ok { memorySz, err := strconv.ParseUint(value, 10, 32) if err != nil { @@ -561,6 +561,9 @@ func addHypervisorMemoryOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig } if value, ok := ocispec.Annotations[vcAnnotations.FileBackedMemRootDir]; ok { + if !regexpContains(runtime.HypervisorConfig.FileBackedMemRootList, value) { + return fmt.Errorf("file_mem_backend value %v required from annotation is not valid", value) + } sbConfig.HypervisorConfig.FileBackedMemRootDir = value } From c16cdcb2a5403d20d63faf50462ba16ccfa0de79 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Tue, 19 May 2020 17:13:09 +0200 Subject: [PATCH 12/24] config: Add makefile variables for path lists Add variables to override defaults at build time for the various lists used to control path annotations. Fixes: #901 Suggested-by: Fabiano Fidencio Signed-off-by: Christophe de Dinechin --- src/runtime/Makefile | 33 +++++++++++++++++++ .../cli/config/configuration-acrn.toml.in | 12 ++----- .../cli/config/configuration-clh.toml.in | 12 ++----- .../cli/config/configuration-fc.toml.in | 12 ++----- .../configuration-qemu-virtiofs.toml.in | 18 ++++------ .../cli/config/configuration-qemu.toml.in | 23 +++++-------- 6 files changed, 57 insertions(+), 53 deletions(-) diff --git a/src/runtime/Makefile b/src/runtime/Makefile index 2a6730d4ef..e8bb5fe9ca 100644 --- a/src/runtime/Makefile +++ b/src/runtime/Makefile @@ -129,16 +129,22 @@ DEFAULT_HYPERVISOR ?= $(HYPERVISOR_QEMU) HYPERVISORS := $(HYPERVISOR_ACRN) $(HYPERVISOR_FC) $(HYPERVISOR_QEMU) $(HYPERVISOR_QEMU_VIRTIOFS) $(HYPERVISOR_CLH) QEMUPATH := $(QEMUBINDIR)/$(QEMUCMD) +QEMUPATHLIST := [\"$(QEMUPATH)\"] QEMUVIRTIOFSPATH := $(QEMUBINDIR)/$(QEMUVIRTIOFSCMD) CLHPATH := $(CLHBINDIR)/$(CLHCMD) +CLHPATHLIST := [\"$(CLHBINDIR)/$(CLHCMD)\"] FCPATH = $(FCBINDIR)/$(FCCMD) +FCPATHLIST = [\"$(FCPATH)\"] FCJAILERPATH = $(FCBINDIR)/$(FCJAILERCMD) +FCJAILERPATHLIST = [\"$(FCJAILERPATH)\"] ACRNPATH := $(ACRNBINDIR)/$(ACRNCMD) +ACRNPATHLIST := [\"$(ACRNPATH)\"] ACRNCTLPATH := $(ACRNBINDIR)/$(ACRNCTLCMD) +ACRNCTLPATHLIST := [\"$(ACRNCTLPATH)\"] SHIMCMD := $(BIN_PREFIX)-shim SHIMPATH := $(PKGLIBEXECDIR)/$(SHIMCMD) @@ -172,6 +178,7 @@ DEFDISABLEBLOCK := false DEFSHAREDFS := virtio-9p DEFSHAREDFS_QEMU_VIRTIOFS := virtio-fs DEFVIRTIOFSDAEMON := $(VIRTIOFSDBINDIR)/virtiofsd +DEFVIRTIOFSDAEMONLIST := [\"$(DEFVIRTIOFSDAEMON)\"] # Default DAX mapping cache size in MiB #if value is 0, DAX is not enabled DEFVIRTIOFSCACHESIZE := 0 @@ -187,6 +194,9 @@ DEFENABLEMEMPREALLOC := false DEFENABLEHUGEPAGES := false DEFENABLEVHOSTUSERSTORE := false DEFVHOSTUSERSTOREPATH := $(PKGRUNDIR)/vhost-user +DEFVHOSTUSERSTOREPATHLIST := [\"$(DEFVHOSTUSERSTOREPATH)\"] +DEFFILEMEMBACKEND := "" +DEFFILEMEMBACKENDLIST := [\"$(DEFFILEMEMBACKEND)\"] DEFENABLESWAP := false DEFENABLEDEBUG := false DEFDISABLENESTINGCHECKS := false @@ -391,10 +401,16 @@ USER_VARS += DEFAULT_HYPERVISOR USER_VARS += ACRNCMD USER_VARS += ACRNCTLCMD USER_VARS += ACRNPATH +USER_VARS += ACRNPATHLIST USER_VARS += ACRNCTLPATH +USER_VARS += ACRNCTLPATHLIST +USER_VARS += CLHPATH +USER_VARS += CLHPATHLIST USER_VARS += FCCMD USER_VARS += FCPATH +USER_VARS += FCPATHLIST USER_VARS += FCJAILERPATH +USER_VARS += FCJAILERPATHLIST USER_VARS += SYSCONFIG USER_VARS += IMAGENAME USER_VARS += IMAGEPATH @@ -425,8 +441,10 @@ USER_VARS += NETMONPATH USER_VARS += QEMUBINDIR USER_VARS += QEMUCMD USER_VARS += QEMUPATH +USER_VARS += QEMUPATHLIST USER_VARS += QEMUVIRTIOFSCMD USER_VARS += QEMUVIRTIOFSPATH +USER_VARS += QEMUVIRTIOFSPATHLIST USER_VARS += SHAREDIR USER_VARS += SHIMPATH USER_VARS += SYSCONFDIR @@ -449,6 +467,7 @@ USER_VARS += DEFBLOCKSTORAGEDRIVER_QEMU_VIRTIOFS USER_VARS += DEFSHAREDFS USER_VARS += DEFSHAREDFS_QEMU_VIRTIOFS USER_VARS += DEFVIRTIOFSDAEMON +USER_VARS += DEFVIRTIOFSDAEMONLIST USER_VARS += DEFVIRTIOFSCACHESIZE USER_VARS += DEFVIRTIOFSCACHE USER_VARS += DEFVIRTIOFSEXTRAARGS @@ -457,6 +476,9 @@ USER_VARS += DEFENABLEMEMPREALLOC USER_VARS += DEFENABLEHUGEPAGES USER_VARS += DEFENABLEVHOSTUSERSTORE USER_VARS += DEFVHOSTUSERSTOREPATH +USER_VARS += DEFVHOSTUSERSTOREPATHLIST +USER_VARS += DEFFILEMEMBACKEND +USER_VARS += DEFFILEMEMBACKENDLIST USER_VARS += DEFENABLESWAP USER_VARS += DEFENABLEDEBUG USER_VARS += DEFDISABLENESTINGCHECKS @@ -606,10 +628,15 @@ $(GENERATED_FILES): %: %.in $(MAKEFILE_LIST) VERSION .git-commit -e "s|@CONFIG_FC_IN@|$(CONFIG_FC_IN)|g" \ -e "s|@CONFIG_PATH@|$(CONFIG_PATH)|g" \ -e "s|@FCPATH@|$(FCPATH)|g" \ + -e "s|@FCPATHLIST@|$(FCPATHLIST)|g" \ -e "s|@FCJAILERPATH@|$(FCJAILERPATH)|g" \ + -e "s|@FCJAILERPATHLIST@|$(FCJAILERPATHLIST)|g" \ -e "s|@ACRNPATH@|$(ACRNPATH)|g" \ + -e "s|@ACRNPATHLIST@|$(ACRNPATHLIST)|g" \ -e "s|@ACRNCTLPATH@|$(ACRNCTLPATH)|g" \ + -e "s|@ACRNCTLPATHLIST@|$(ACRNCTLPATHLIST)|g" \ -e "s|@CLHPATH@|$(CLHPATH)|g" \ + -e "s|@CLHPATHLIST@|$(CLHPATHLIST)|g" \ -e "s|@SYSCONFIG@|$(SYSCONFIG)|g" \ -e "s|@IMAGEPATH@|$(IMAGEPATH)|g" \ -e "s|@KERNELPATH_ACRN@|$(KERNELPATH_ACRN)|g" \ @@ -635,7 +662,9 @@ $(GENERATED_FILES): %: %.in $(MAKEFILE_LIST) VERSION .git-commit -e "s|@PROJECT_TAG@|$(PROJECT_TAG)|g" \ -e "s|@PROJECT_TYPE@|$(PROJECT_TYPE)|g" \ -e "s|@QEMUPATH@|$(QEMUPATH)|g" \ + -e "s|@QEMUPATHLIST@|$(QEMUPATHLIST)|g" \ -e "s|@QEMUVIRTIOFSPATH@|$(QEMUVIRTIOFSPATH)|g" \ + -e "s|@QEMUVIRTIOFSPATHLIST@|$(QEMUVIRTIOFSPATHLIST)|g" \ -e "s|@RUNTIME_NAME@|$(TARGET)|g" \ -e "s|@MACHINETYPE@|$(MACHINETYPE)|g" \ -e "s|@SHIMPATH@|$(SHIMPATH)|g" \ @@ -659,6 +688,7 @@ $(GENERATED_FILES): %: %.in $(MAKEFILE_LIST) VERSION .git-commit -e "s|@DEFSHAREDFS@|$(DEFSHAREDFS)|g" \ -e "s|@DEFSHAREDFS_QEMU_VIRTIOFS@|$(DEFSHAREDFS_QEMU_VIRTIOFS)|g" \ -e "s|@DEFVIRTIOFSDAEMON@|$(DEFVIRTIOFSDAEMON)|g" \ + -e "s|@DEFVIRTIOFSDAEMONLIST@|$(DEFVIRTIOFSDAEMONLIST)|g" \ -e "s|@DEFVIRTIOFSCACHESIZE@|$(DEFVIRTIOFSCACHESIZE)|g" \ -e "s|@DEFVIRTIOFSCACHE@|$(DEFVIRTIOFSCACHE)|g" \ -e "s|@DEFVIRTIOFSEXTRAARGS@|$(DEFVIRTIOFSEXTRAARGS)|g" \ @@ -667,6 +697,9 @@ $(GENERATED_FILES): %: %.in $(MAKEFILE_LIST) VERSION .git-commit -e "s|@DEFENABLEHUGEPAGES@|$(DEFENABLEHUGEPAGES)|g" \ -e "s|@DEFENABLEVHOSTUSERSTORE@|$(DEFENABLEVHOSTUSERSTORE)|g" \ -e "s|@DEFVHOSTUSERSTOREPATH@|$(DEFVHOSTUSERSTOREPATH)|g" \ + -e "s|@DEFVHOSTUSERSTOREPATHLIST@|$(DEFVHOSTUSERSTOREPATHLIST)|g" \ + -e "s|@DEFFILEMEMBACKEND@|$(DEFFILEMEMBACKEND)|g" \ + -e "s|@DEFFILEMEMBACKENDLIST@|$(DEFFILEMEMBACKENDLIST)|g" \ -e "s|@DEFENABLEMSWAP@|$(DEFENABLESWAP)|g" \ -e "s|@DEFENABLEDEBUG@|$(DEFENABLEDEBUG)|g" \ -e "s|@DEFDISABLENESTINGCHECKS@|$(DEFDISABLENESTINGCHECKS)|g" \ diff --git a/src/runtime/cli/config/configuration-acrn.toml.in b/src/runtime/cli/config/configuration-acrn.toml.in index d56b2ec9bb..7478fed93d 100644 --- a/src/runtime/cli/config/configuration-acrn.toml.in +++ b/src/runtime/cli/config/configuration-acrn.toml.in @@ -17,17 +17,11 @@ kernel = "@KERNELPATH_ACRN@" image = "@IMAGEPATH@" # List of valid annotations values for the hypervisor (default: empty) -# Each member of the list can be a regular expression, but prefer names. -# Otherwise, please read and understand the following carefully. -# SECURITY WARNING: If you use regular expressions, be mindful that -# an attacker could craft an annotation that uses .. to escape the paths -# you gave. For example, if your regexp is /bin/qemu.* then if there is -# a directory named /bin/qemu.d/, then an attacker can pass an annotation -# containing /bin/qemu.d/../put-any-binary-name-here and attack your host. -# path_list = [ "@ACRNPATH@.*" ] +# Each member of the list is a path pattern as described by glob(3). +path_list = @ACRNPATHLIST@ # List of valid annotations values for ctlpath (default: empty) -# ctlpath_list = [ "@ACRNCTLPATH@.*" ] +ctlpath_list = @ACRNCTLPATHLIST@ # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having diff --git a/src/runtime/cli/config/configuration-clh.toml.in b/src/runtime/cli/config/configuration-clh.toml.in index 8e2419f62e..f1f02a129b 100644 --- a/src/runtime/cli/config/configuration-clh.toml.in +++ b/src/runtime/cli/config/configuration-clh.toml.in @@ -16,14 +16,8 @@ kernel = "@KERNELPATH_CLH@" image = "@IMAGEPATH@" # List of valid annotations values for the hypervisor (default: empty) -# Each member of the list can be a regular expression, but prefer names. -# Otherwise, please read and understand the following carefully. -# SECURITY WARNING: If you use regular expressions, be mindful that -# an attacker could craft an annotation that uses .. to escape the paths -# you gave. For example, if your regexp is /bin/qemu.* then if there is -# a directory named /bin/qemu.d/, then an attacker can pass an annotation -# containing /bin/qemu.d/../put-any-binary-name-here and attack your host. -# path_list = [ "@CLHPATH@.*" ] +# Each member of the list is a path pattern as described by glob(3). +path_list = @CLHPATHLIST@ # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having @@ -73,7 +67,7 @@ default_memory = @DEFMEMSZ@ virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" # List of valid annotations values for the virtiofs daemon (default: empty) -# virtio_fs_daemon_list = [ "/opt/kata/bin/virtiofsd", "/usr/.*/virtiofsd" ] +virtio_fs_daemon_list = @DEFVIRTIOFSDAEMONLIST@ # Default size of DAX cache in MiB virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ diff --git a/src/runtime/cli/config/configuration-fc.toml.in b/src/runtime/cli/config/configuration-fc.toml.in index 2c239cd4e7..b6fd8647d5 100644 --- a/src/runtime/cli/config/configuration-fc.toml.in +++ b/src/runtime/cli/config/configuration-fc.toml.in @@ -16,14 +16,8 @@ kernel = "@KERNELPATH_FC@" image = "@IMAGEPATH@" # List of valid annotations values for the hypervisor (default: empty) -# Each member of the list can be a regular expression, but prefer names. -# Otherwise, please read and understand the following carefully. -# SECURITY WARNING: If you use regular expressions, be mindful that -# an attacker could craft an annotation that uses .. to escape the paths -# you gave. For example, if your regexp is /bin/qemu.* then if there is -# a directory named /bin/qemu.d/, then an attacker can pass an annotation -# containing /bin/qemu.d/../put-any-binary-name-here and attack your host. -# path_list = [ "@FCPATH@.*" ] +# Each member of the list is a path pattern as described by glob(3). +path_list = @FCPATHLIST@ # Path for the jailer specific to firecracker # If the jailer path is not set kata will launch firecracker @@ -35,7 +29,7 @@ image = "@IMAGEPATH@" # List of valid jailer path values for the hypervisor (default: empty) # Each member of the list can be a regular expression -# jailer_path_list = [ "@FCJAILERPATH@.*" ] +# jailer_path_list = @FCJAILERPATHLIST@ # Optional space-separated list of options to pass to the guest kernel. diff --git a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in index d9564adfb3..22ec8d91e1 100644 --- a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in +++ b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in @@ -17,14 +17,8 @@ image = "@IMAGEPATH@" machine_type = "@MACHINETYPE@" # List of valid annotations values for the hypervisor (default: empty) -# Each member of the list can be a regular expression, but prefer names. -# Otherwise, please read and understand the following carefully. -# SECURITY WARNING: If you use regular expressions, be mindful that -# an attacker could craft an annotation that uses .. to escape the paths -# you gave. For example, if your regexp is /bin/qemu.* then if there is -# a directory named /bin/qemu.d/, then an attacker can pass an annotation -# containing /bin/qemu.d/../put-any-binary-name-here and attack your host. -# path_list = [ "@QEMUPATH@.*" ] +# Each member of the list is a path pattern as described by glob(3). +path_list = @QEMUVIRTIOFSPATHLIST@ # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having @@ -121,7 +115,7 @@ shared_fs = "@DEFSHAREDFS_QEMU_VIRTIOFS@" virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" # List of valid annotations values for the virtiofs daemon (default: empty) -# virtio_fs_daemon_list = [ "/opt/kata/bin/virtiofsd", "/usr/.*/virtiofsd" ] +virtio_fs_daemon_list = @DEFVIRTIOFSDAEMONLIST@ # Default size of DAX cache in MiB virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ @@ -213,16 +207,16 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" #enable_iommu_platform = true # List of valid annotations values for the virtiofs daemon (default: empty) -# vhost_user_store_path_list = [ "/empty/space", "/multiverse/quantum-foam" ] +vhost_user_store_path_list = @DEFVHOSTUSERSTOREPATHLIST@ # Enable file based guest memory support. The default is an empty string which # will disable this feature. In the case of virtio-fs, this is enabled # automatically and '/dev/shm' is used as the backing folder. # This option will be ignored if VM templating is enabled. -#file_mem_backend = "" +#file_mem_backend = "@DEFFILEMEMBACKEND@" # List of valid annotations values for the file_mem_backend annotation (default: empty) -# file_mem_backend_list = [ "/dev/shm" ] +#file_mem_backend_list = @DEFFILEMEMBACKENDLIST@ # Enable swap of vm memory. Default false. # The behaviour is undefined if mem_prealloc is also set to true diff --git a/src/runtime/cli/config/configuration-qemu.toml.in b/src/runtime/cli/config/configuration-qemu.toml.in index e87daa107e..9891d2002c 100644 --- a/src/runtime/cli/config/configuration-qemu.toml.in +++ b/src/runtime/cli/config/configuration-qemu.toml.in @@ -12,19 +12,14 @@ [hypervisor.qemu] path = "@QEMUPATH@" -# List of valid annotations values for the hypervisor (default: empty) -# Each member of the list can be a regular expression, but prefer names. -# Otherwise, please read and understand the following carefully. -# SECURITY WARNING: If you use regular expressions, be mindful that -# an attacker could craft an annotation that uses .. to escape the paths -# you gave. For example, if your regexp is /bin/qemu.* then if there is -# a directory named /bin/qemu.d/, then an attacker can pass an annotation -# containing /bin/qemu.d/../put-any-binary-name-here and attack your host. -# path_list = [ "@QEMUPATH@.*" ] kernel = "@KERNELPATH@" image = "@IMAGEPATH@" machine_type = "@MACHINETYPE@" +# List of valid annotations values for the hypervisor (default: empty) +# Each member of the list is a path pattern as described by glob(3). +path_list = @QEMUPATHLIST@ + # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having # trouble running pre-2.15 glibc. @@ -126,7 +121,7 @@ shared_fs = "@DEFSHAREDFS@" virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" # List of valid annotations values for the virtiofs daemon (default: empty) -# virtio_fs_daemon_list = [ "/opt/kata/bin/virtiofsd", "/usr/.*/virtiofsd" ] +virtio_fs_daemon_list = @DEFVIRTIOFSDAEMONLIST@ # Default size of DAX cache in MiB virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ @@ -217,17 +212,17 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # Enabling this will result in the VM device having iommu_platform=on set #enable_iommu_platform = true -# List of valid annotations values for the virtiofs daemon (default: empty) -# vhost_user_store_path_list = [ "/empty/space", "/multiverse/quantum-foam" ] +# List of valid annotations values for the vhost user store path (default: empty) +vhost_user_store_path_list = @DEFVHOSTUSERSTOREPATHLIST@ # Enable file based guest memory support. The default is an empty string which # will disable this feature. In the case of virtio-fs, this is enabled # automatically and '/dev/shm' is used as the backing folder. # This option will be ignored if VM templating is enabled. -#file_mem_backend = "" +#file_mem_backend = "@DEFFILEMEMBACKEND@" # List of valid annotations values for the file_mem_backend annotation (default: empty) -# file_mem_backend_list = [ "/dev/shm" ] +#file_mem_backend_list = @DEFFILEMEMBACKENDLIST@ # Enable swap of vm memory. Default false. # The behaviour is undefined if mem_prealloc is also set to true From 11b9c90cd812dfdbe4f7501f821bfdc045d100dd Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Tue, 19 May 2020 17:52:32 +0200 Subject: [PATCH 13/24] annotations: Fix typo in comment A comment talking about runtime related annotations describes them as being related to the agent. A similar comment for the agent annotations is missing. Fixes: #901 Signed-off-by: Christophe de Dinechin --- src/runtime/virtcontainers/pkg/annotations/annotations.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/runtime/virtcontainers/pkg/annotations/annotations.go b/src/runtime/virtcontainers/pkg/annotations/annotations.go index 5e8f52c503..52f8256391 100644 --- a/src/runtime/virtcontainers/pkg/annotations/annotations.go +++ b/src/runtime/virtcontainers/pkg/annotations/annotations.go @@ -214,7 +214,7 @@ const ( TxRateLimiterMaxRate = kataAnnotHypervisorPrefix + "tx_rate_limiter_max_rate" ) -// Agent related annotations +// Runtime related annotations const ( kataAnnotRuntimePrefix = kataConfAnnotationsPrefix + "runtime." @@ -238,6 +238,7 @@ const ( DisableNewNetNs = kataAnnotRuntimePrefix + "disable_new_netns" ) +// Agent related annotations const ( kataAnnotAgentPrefix = kataConfAnnotationsPrefix + "agent." From f047fced0b870c39dc175a0ee4c4b9904ff32257 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Mon, 18 May 2020 19:00:54 +0200 Subject: [PATCH 14/24] config: Use glob instead of regexp to match paths in annotations When filtering annotations that correspond to paths, e.g. hypervisor.path, it is better to use a glob syntax than a regexp syntax, as it is more usual for paths, and prevents classes of matches that are undesirable in our case, such as matching .. against .* Fixes: #901 Signed-off-by: Christophe de Dinechin --- src/runtime/virtcontainers/pkg/oci/utils.go | 24 +++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index 2053d4d403..70e780701c 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -201,6 +201,18 @@ func regexpContains(s []string, e string) bool { return false } +func checkPathIsInGlobList(list []string, path string) bool { + for _, glob := range list { + filenames, _ := filepath.Glob(glob) + for _, a := range filenames { + if path == a { + return true + } + } + } + return false +} + func newLinuxDeviceInfo(d specs.LinuxDevice) (*config.DeviceInfo, error) { allowedDeviceTypes := []string{"c", "b", "u", "p"} @@ -392,21 +404,21 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, } if value, ok := ocispec.Annotations[vcAnnotations.HypervisorPath]; ok { - if !regexpContains(runtime.HypervisorConfig.HypervisorPathList, value) { + if !checkPathIsInGlobList(runtime.HypervisorConfig.HypervisorPathList, value) { return fmt.Errorf("hypervisor %v required from annotation is not valid", value) } config.HypervisorConfig.HypervisorPath = value } if value, ok := ocispec.Annotations[vcAnnotations.JailerPath]; ok { - if !regexpContains(runtime.HypervisorConfig.JailerPathList, value) { + if !checkPathIsInGlobList(runtime.HypervisorConfig.JailerPathList, value) { return fmt.Errorf("jailer %v required from annotation is not valid", value) } config.HypervisorConfig.JailerPath = value } if value, ok := ocispec.Annotations[vcAnnotations.CtlPath]; ok { - if !regexpContains(runtime.HypervisorConfig.HypervisorCtlPathList, value) { + if !checkPathIsInGlobList(runtime.HypervisorConfig.HypervisorCtlPathList, value) { return fmt.Errorf("hypervisor control %v required from annotation is not valid", value) } config.HypervisorConfig.HypervisorCtlPath = value @@ -436,7 +448,7 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, } if value, ok := ocispec.Annotations[vcAnnotations.VhostUserStorePath]; ok { - if !regexpContains(runtime.HypervisorConfig.VhostUserStorePathList, value) { + if !checkPathIsInGlobList(runtime.HypervisorConfig.VhostUserStorePathList, value) { return fmt.Errorf("vhost store path %v required from annotation is not valid", value) } config.HypervisorConfig.VhostUserStorePath = value @@ -561,7 +573,7 @@ func addHypervisorMemoryOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig } if value, ok := ocispec.Annotations[vcAnnotations.FileBackedMemRootDir]; ok { - if !regexpContains(runtime.HypervisorConfig.FileBackedMemRootList, value) { + if !checkPathIsInGlobList(runtime.HypervisorConfig.FileBackedMemRootList, value) { return fmt.Errorf("file_mem_backend value %v required from annotation is not valid", value) } sbConfig.HypervisorConfig.FileBackedMemRootDir = value @@ -717,7 +729,7 @@ func addHypervisorVirtioFsOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConf } if value, ok := ocispec.Annotations[vcAnnotations.VirtioFSDaemon]; ok { - if !regexpContains(runtime.HypervisorConfig.VirtioFSDaemonList, value) { + if !checkPathIsInGlobList(runtime.HypervisorConfig.VirtioFSDaemonList, value) { return fmt.Errorf("virtiofs daemon %v required from annotation is not valid", value) } sbConfig.HypervisorConfig.VirtioFSDaemon = value From 7c6aede5d4c296468652e85d039385da8c026d7e Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Tue, 19 May 2020 19:19:42 +0200 Subject: [PATCH 15/24] config: Whitelist hypervisor annotations by name Add a field "enable_annotations" to the runtime configuration that can be used to whitelist annotations using a list of regular expressions, which are used to match any part of the base annotation name, i.e. the part after "io.katacontainers.config.hypervisor." For example, the following configuraiton will match "virtio_fs_daemon", "initrd" and "jailer_path", but not "path" nor "firmware": enable_annotations = [ "virtio.*", "initrd", "_path" ] The default is an empty list of enabled annotations, which disables annotations entirely. If an anontation is rejected, the message is something like: annotation io.katacontainers.config.hypervisor.virtio_fs_daemon is not enabled Fixes: #901 Suggested-by: Peng Tao Signed-off-by: Christophe de Dinechin --- src/runtime/Makefile | 2 ++ src/runtime/cli/config/configuration-acrn.toml.in | 5 +++++ src/runtime/cli/config/configuration-clh.toml.in | 5 +++++ src/runtime/cli/config/configuration-fc.toml.in | 5 +++++ .../cli/config/configuration-qemu-virtiofs.toml.in | 5 +++++ src/runtime/cli/config/configuration-qemu.toml.in | 5 +++++ src/runtime/pkg/katautils/config.go | 5 +++++ src/runtime/virtcontainers/hypervisor.go | 3 +++ src/runtime/virtcontainers/persist.go | 2 ++ src/runtime/virtcontainers/persist/api/config.go | 3 +++ .../virtcontainers/pkg/annotations/annotations.go | 1 + src/runtime/virtcontainers/pkg/oci/utils.go | 13 +++++++++++++ 12 files changed, 54 insertions(+) diff --git a/src/runtime/Makefile b/src/runtime/Makefile index e8bb5fe9ca..784c4c705c 100644 --- a/src/runtime/Makefile +++ b/src/runtime/Makefile @@ -167,6 +167,7 @@ DEFMEMSZ := 2048 DEFMEMSLOTS := 10 #Default number of bridges DEFBRIDGES := 1 +DEFENABLEANNOTATIONS := [] DEFDISABLEGUESTSECCOMP := true #Default experimental features enabled DEFAULTEXPFEATURES := [] @@ -678,6 +679,7 @@ $(GENERATED_FILES): %: %.in $(MAKEFILE_LIST) VERSION .git-commit -e "s|@DEFNETWORKMODEL_CLH@|$(DEFNETWORKMODEL_CLH)|g" \ -e "s|@DEFNETWORKMODEL_FC@|$(DEFNETWORKMODEL_FC)|g" \ -e "s|@DEFNETWORKMODEL_QEMU@|$(DEFNETWORKMODEL_QEMU)|g" \ + -e "s|@DEFENABLEANNOTATIONS@|$(DEFENABLEANNOTATIONS)|g" \ -e "s|@DEFDISABLEGUESTSECCOMP@|$(DEFDISABLEGUESTSECCOMP)|g" \ -e "s|@DEFAULTEXPFEATURES@|$(DEFAULTEXPFEATURES)|g" \ -e "s|@DEFDISABLEBLOCK@|$(DEFDISABLEBLOCK)|g" \ diff --git a/src/runtime/cli/config/configuration-acrn.toml.in b/src/runtime/cli/config/configuration-acrn.toml.in index 7478fed93d..3077495277 100644 --- a/src/runtime/cli/config/configuration-acrn.toml.in +++ b/src/runtime/cli/config/configuration-acrn.toml.in @@ -16,6 +16,11 @@ ctlpath = "@ACRNCTLPATH@" kernel = "@KERNELPATH_ACRN@" image = "@IMAGEPATH@" +# List of valid annotation names for the hypervisor +# Each member of the list is a regular expression, which is the base name +# of the annotation, e.g. "path" for io.katacontainers.config.hypervisor.path" +enable_annotations = @DEFENABLEANNOTATIONS@ + # List of valid annotations values for the hypervisor (default: empty) # Each member of the list is a path pattern as described by glob(3). path_list = @ACRNPATHLIST@ diff --git a/src/runtime/cli/config/configuration-clh.toml.in b/src/runtime/cli/config/configuration-clh.toml.in index f1f02a129b..0dde89c851 100644 --- a/src/runtime/cli/config/configuration-clh.toml.in +++ b/src/runtime/cli/config/configuration-clh.toml.in @@ -15,6 +15,11 @@ path = "@CLHPATH@" kernel = "@KERNELPATH_CLH@" image = "@IMAGEPATH@" +# List of valid annotation names for the hypervisor +# Each member of the list is a regular expression, which is the base name +# of the annotation, e.g. "path" for io.katacontainers.config.hypervisor.path" +enable_annotations = @DEFENABLEANNOTATIONS@ + # List of valid annotations values for the hypervisor (default: empty) # Each member of the list is a path pattern as described by glob(3). path_list = @CLHPATHLIST@ diff --git a/src/runtime/cli/config/configuration-fc.toml.in b/src/runtime/cli/config/configuration-fc.toml.in index b6fd8647d5..403928886f 100644 --- a/src/runtime/cli/config/configuration-fc.toml.in +++ b/src/runtime/cli/config/configuration-fc.toml.in @@ -15,6 +15,11 @@ path = "@FCPATH@" kernel = "@KERNELPATH_FC@" image = "@IMAGEPATH@" +# List of valid annotation names for the hypervisor +# Each member of the list is a regular expression, which is the base name +# of the annotation, e.g. "path" for io.katacontainers.config.hypervisor.path" +enable_annotations = @DEFENABLEANNOTATIONS@ + # List of valid annotations values for the hypervisor (default: empty) # Each member of the list is a path pattern as described by glob(3). path_list = @FCPATHLIST@ diff --git a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in index 22ec8d91e1..afde7e5f90 100644 --- a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in +++ b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in @@ -16,6 +16,11 @@ kernel = "@KERNELVIRTIOFSPATH@" image = "@IMAGEPATH@" machine_type = "@MACHINETYPE@" +# List of valid annotation names for the hypervisor +# Each member of the list is a regular expression, which is the base name +# of the annotation, e.g. "path" for io.katacontainers.config.hypervisor.path" +enable_annotations = @DEFENABLEANNOTATIONS@ + # List of valid annotations values for the hypervisor (default: empty) # Each member of the list is a path pattern as described by glob(3). path_list = @QEMUVIRTIOFSPATHLIST@ diff --git a/src/runtime/cli/config/configuration-qemu.toml.in b/src/runtime/cli/config/configuration-qemu.toml.in index 9891d2002c..cb52589377 100644 --- a/src/runtime/cli/config/configuration-qemu.toml.in +++ b/src/runtime/cli/config/configuration-qemu.toml.in @@ -16,6 +16,11 @@ kernel = "@KERNELPATH@" image = "@IMAGEPATH@" machine_type = "@MACHINETYPE@" +# List of valid annotation names for the hypervisor +# Each member of the list is a regular expression, which is the base name +# of the annotation, e.g. "path" for io.katacontainers.config.hypervisor.path" +enable_annotations = @DEFENABLEANNOTATIONS@ + # List of valid annotations values for the hypervisor (default: empty) # Each member of the list is a path pattern as described by glob(3). path_list = @QEMUPATHLIST@ diff --git a/src/runtime/pkg/katautils/config.go b/src/runtime/pkg/katautils/config.go index d266bd55fd..12cf42438d 100644 --- a/src/runtime/pkg/katautils/config.go +++ b/src/runtime/pkg/katautils/config.go @@ -124,6 +124,7 @@ type hypervisor struct { GuestHookPath string `toml:"guest_hook_path"` RxRateLimiterMaxRate uint64 `toml:"rx_rate_limiter_max_rate"` TxRateLimiterMaxRate uint64 `toml:"tx_rate_limiter_max_rate"` + EnableAnnotations []string `toml:"enable_annotations"` } type runtime struct { @@ -558,6 +559,7 @@ func newFirecrackerHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { GuestHookPath: h.guestHookPath(), RxRateLimiterMaxRate: rxRateLimiterMaxRate, TxRateLimiterMaxRate: txRateLimiterMaxRate, + EnableAnnotations: h.EnableAnnotations, }, nil } @@ -685,6 +687,7 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { GuestHookPath: h.guestHookPath(), RxRateLimiterMaxRate: rxRateLimiterMaxRate, TxRateLimiterMaxRate: txRateLimiterMaxRate, + EnableAnnotations: h.EnableAnnotations, }, nil } @@ -748,6 +751,7 @@ func newAcrnHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { BlockDeviceDriver: blockDriver, DisableVhostNet: h.DisableVhostNet, GuestHookPath: h.guestHookPath(), + EnableAnnotations: h.EnableAnnotations, }, nil } @@ -840,6 +844,7 @@ func newClhHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { DisableVhostNet: true, VirtioFSExtraArgs: h.VirtioFSExtraArgs, SGXEPCSize: defaultSGXEPCSize, + EnableAnnotations: h.EnableAnnotations, }, nil } diff --git a/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index 46fac8907d..c67971001a 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/virtcontainers/hypervisor.go @@ -437,6 +437,9 @@ type HypervisorConfig struct { // SGXEPCSize specifies the size in bytes for the EPC Section. // Enable SGX. Hardware-based isolation and memory encryption. SGXEPCSize int64 + + // Enable annotations by name + EnableAnnotations []string } // vcpu mapping from vcpu number to thread number diff --git a/src/runtime/virtcontainers/persist.go b/src/runtime/virtcontainers/persist.go index 65399e86af..78c200bc99 100644 --- a/src/runtime/virtcontainers/persist.go +++ b/src/runtime/virtcontainers/persist.go @@ -254,6 +254,7 @@ func (s *Sandbox) dumpConfig(ss *persistapi.SandboxState) { RxRateLimiterMaxRate: sconfig.HypervisorConfig.RxRateLimiterMaxRate, TxRateLimiterMaxRate: sconfig.HypervisorConfig.TxRateLimiterMaxRate, SGXEPCSize: sconfig.HypervisorConfig.SGXEPCSize, + EnableAnnotations: sconfig.HypervisorConfig.EnableAnnotations, } ss.Config.KataAgentConfig = &persistapi.KataAgentConfig{ @@ -522,6 +523,7 @@ func loadSandboxConfig(id string) (*SandboxConfig, error) { RxRateLimiterMaxRate: hconf.RxRateLimiterMaxRate, TxRateLimiterMaxRate: hconf.TxRateLimiterMaxRate, SGXEPCSize: hconf.SGXEPCSize, + EnableAnnotations: hconf.EnableAnnotations, } sconfig.AgentConfig = KataAgentConfig{ diff --git a/src/runtime/virtcontainers/persist/api/config.go b/src/runtime/virtcontainers/persist/api/config.go index 025c93d1a9..732cbec7e4 100644 --- a/src/runtime/virtcontainers/persist/api/config.go +++ b/src/runtime/virtcontainers/persist/api/config.go @@ -208,6 +208,9 @@ type HypervisorConfig struct { // SGXEPCSize specifies the size in bytes for the EPC Section. // Enable SGX. Hardware-based isolation and memory encryption. SGXEPCSize int64 + + // Enable annotations by name + EnableAnnotations []string } // KataAgentConfig is a structure storing information needed diff --git a/src/runtime/virtcontainers/pkg/annotations/annotations.go b/src/runtime/virtcontainers/pkg/annotations/annotations.go index 52f8256391..d044e8891f 100644 --- a/src/runtime/virtcontainers/pkg/annotations/annotations.go +++ b/src/runtime/virtcontainers/pkg/annotations/annotations.go @@ -28,6 +28,7 @@ const ( // // Assets // + KataAnnotationHypervisorPrefix = kataAnnotHypervisorPrefix // KernelPath is a sandbox annotation for passing a per container path pointing at the kernel needed to boot the container VM. KernelPath = kataAnnotHypervisorPrefix + "kernel" diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index 70e780701c..5d02f5a378 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -213,6 +213,14 @@ func checkPathIsInGlobList(list []string, path string) bool { return false } +// Check if an annotation name either belongs to another prefix, matches regexp list +func checkAnnotationNameIsValid(list []string, name string, prefix string) bool { + if strings.HasPrefix(name, prefix) { + return regexpContains(list, strings.TrimPrefix(name, prefix)) + } + return true +} + func newLinuxDeviceInfo(d specs.LinuxDevice) (*config.DeviceInfo, error) { allowedDeviceTypes := []string{"c", "b", "u", "p"} @@ -346,6 +354,11 @@ func SandboxID(spec specs.Spec) (string, error) { } func addAnnotations(ocispec specs.Spec, config *vc.SandboxConfig, runtime RuntimeConfig) error { + for key := range ocispec.Annotations { + if !checkAnnotationNameIsValid(runtime.HypervisorConfig.EnableAnnotations, key, vcAnnotations.KataAnnotationHypervisorPrefix) { + return fmt.Errorf("annotation %v is not enabled", key) + } + } addAssetAnnotations(ocispec, config) if err := addHypervisorConfigOverrides(ocispec, config, runtime); err != nil { return err From d65a7d1083dc5a60faeca277bd99768350364176 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 3 Sep 2020 13:42:12 +0200 Subject: [PATCH 16/24] config: Add better comments in the template files When there is a default value from the code (usually empty) that differs from a possible suggested value from the distro, then the wording "default: empty" is confusing. Fixes: #901 Suggested-by: Julio Montes Signed-off-by: Christophe de Dinechin --- .../cli/config/configuration-acrn.toml.in | 8 ++++++-- .../cli/config/configuration-clh.toml.in | 8 ++++++-- .../cli/config/configuration-fc.toml.in | 10 +++++++--- .../configuration-qemu-virtiofs.toml.in | 20 ++++++++++++++----- .../cli/config/configuration-qemu.toml.in | 18 ++++++++++++----- 5 files changed, 47 insertions(+), 17 deletions(-) diff --git a/src/runtime/cli/config/configuration-acrn.toml.in b/src/runtime/cli/config/configuration-acrn.toml.in index 3077495277..85d60cd443 100644 --- a/src/runtime/cli/config/configuration-acrn.toml.in +++ b/src/runtime/cli/config/configuration-acrn.toml.in @@ -21,11 +21,15 @@ image = "@IMAGEPATH@" # of the annotation, e.g. "path" for io.katacontainers.config.hypervisor.path" enable_annotations = @DEFENABLEANNOTATIONS@ -# List of valid annotations values for the hypervisor (default: empty) +# List of valid annotations values for the hypervisor # Each member of the list is a path pattern as described by glob(3). +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @ACRNPATHLIST@ path_list = @ACRNPATHLIST@ -# List of valid annotations values for ctlpath (default: empty) +# List of valid annotations values for ctlpath +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @ACRNCTLPATHLIST@ ctlpath_list = @ACRNCTLPATHLIST@ # Optional space-separated list of options to pass to the guest kernel. diff --git a/src/runtime/cli/config/configuration-clh.toml.in b/src/runtime/cli/config/configuration-clh.toml.in index 0dde89c851..33379c8852 100644 --- a/src/runtime/cli/config/configuration-clh.toml.in +++ b/src/runtime/cli/config/configuration-clh.toml.in @@ -20,8 +20,10 @@ image = "@IMAGEPATH@" # of the annotation, e.g. "path" for io.katacontainers.config.hypervisor.path" enable_annotations = @DEFENABLEANNOTATIONS@ -# List of valid annotations values for the hypervisor (default: empty) +# List of valid annotations values for the hypervisor # Each member of the list is a path pattern as described by glob(3). +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @CLHPATHLIST@ path_list = @CLHPATHLIST@ # Optional space-separated list of options to pass to the guest kernel. @@ -71,7 +73,9 @@ default_memory = @DEFMEMSZ@ # Path to vhost-user-fs daemon. virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" -# List of valid annotations values for the virtiofs daemon (default: empty) +# List of valid annotations values for the virtiofs daemon +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @DEFVIRTIOFSDAEMONLIST@ virtio_fs_daemon_list = @DEFVIRTIOFSDAEMONLIST@ # Default size of DAX cache in MiB diff --git a/src/runtime/cli/config/configuration-fc.toml.in b/src/runtime/cli/config/configuration-fc.toml.in index 403928886f..e21d2c1ddc 100644 --- a/src/runtime/cli/config/configuration-fc.toml.in +++ b/src/runtime/cli/config/configuration-fc.toml.in @@ -20,8 +20,10 @@ image = "@IMAGEPATH@" # of the annotation, e.g. "path" for io.katacontainers.config.hypervisor.path" enable_annotations = @DEFENABLEANNOTATIONS@ -# List of valid annotations values for the hypervisor (default: empty) +# List of valid annotations values for the hypervisor # Each member of the list is a path pattern as described by glob(3). +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @FCPATHLIST@ path_list = @FCPATHLIST@ # Path for the jailer specific to firecracker @@ -32,9 +34,11 @@ path_list = @FCPATHLIST@ # for this feature today. #jailer_path = "@FCJAILERPATH@" -# List of valid jailer path values for the hypervisor (default: empty) +# List of valid jailer path values for the hypervisor # Each member of the list can be a regular expression -# jailer_path_list = @FCJAILERPATHLIST@ +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @FCJAILERPATHLIST@ +jailer_path_list = @FCJAILERPATHLIST@ # Optional space-separated list of options to pass to the guest kernel. diff --git a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in index afde7e5f90..27c963d749 100644 --- a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in +++ b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in @@ -19,10 +19,14 @@ machine_type = "@MACHINETYPE@" # List of valid annotation names for the hypervisor # Each member of the list is a regular expression, which is the base name # of the annotation, e.g. "path" for io.katacontainers.config.hypervisor.path" +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @DEFENABLEANNOTATIONS@ enable_annotations = @DEFENABLEANNOTATIONS@ -# List of valid annotations values for the hypervisor (default: empty) +# List of valid annotations values for the hypervisor # Each member of the list is a path pattern as described by glob(3). +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @QEMUVIRTIOFSPATHLIST@ path_list = @QEMUVIRTIOFSPATHLIST@ # Optional space-separated list of options to pass to the guest kernel. @@ -119,7 +123,9 @@ shared_fs = "@DEFSHAREDFS_QEMU_VIRTIOFS@" # Path to vhost-user-fs daemon. virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" -# List of valid annotations values for the virtiofs daemon (default: empty) +# List of valid annotations values for the virtiofs daemon +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @DEFVIRTIOFSDAEMONLIST@ virtio_fs_daemon_list = @DEFVIRTIOFSDAEMONLIST@ # Default size of DAX cache in MiB @@ -211,7 +217,9 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # Enabling this will result in the VM device having iommu_platform=on set #enable_iommu_platform = true -# List of valid annotations values for the virtiofs daemon (default: empty) +# List of valid annotations values for the virtiofs daemon +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @DEFVHOSTUSERSTOREPATHLIST@ vhost_user_store_path_list = @DEFVHOSTUSERSTOREPATHLIST@ # Enable file based guest memory support. The default is an empty string which @@ -220,8 +228,10 @@ vhost_user_store_path_list = @DEFVHOSTUSERSTOREPATHLIST@ # This option will be ignored if VM templating is enabled. #file_mem_backend = "@DEFFILEMEMBACKEND@" -# List of valid annotations values for the file_mem_backend annotation (default: empty) -#file_mem_backend_list = @DEFFILEMEMBACKENDLIST@ +# List of valid annotations values for the file_mem_backend annotation +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @DEFFILEMEMBACKENDLIST@ +file_mem_backend_list = @DEFFILEMEMBACKENDLIST@ # Enable swap of vm memory. Default false. # The behaviour is undefined if mem_prealloc is also set to true diff --git a/src/runtime/cli/config/configuration-qemu.toml.in b/src/runtime/cli/config/configuration-qemu.toml.in index cb52589377..87b10d3175 100644 --- a/src/runtime/cli/config/configuration-qemu.toml.in +++ b/src/runtime/cli/config/configuration-qemu.toml.in @@ -21,8 +21,10 @@ machine_type = "@MACHINETYPE@" # of the annotation, e.g. "path" for io.katacontainers.config.hypervisor.path" enable_annotations = @DEFENABLEANNOTATIONS@ -# List of valid annotations values for the hypervisor (default: empty) +# List of valid annotations values for the hypervisor # Each member of the list is a path pattern as described by glob(3). +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @QEMUPATHLIST@ path_list = @QEMUPATHLIST@ # Optional space-separated list of options to pass to the guest kernel. @@ -125,7 +127,9 @@ shared_fs = "@DEFSHAREDFS@" # Path to vhost-user-fs daemon. virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" -# List of valid annotations values for the virtiofs daemon (default: empty) +# List of valid annotations values for the virtiofs daemon +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @DEFVIRTIOFSDAEMONLIST@ virtio_fs_daemon_list = @DEFVIRTIOFSDAEMONLIST@ # Default size of DAX cache in MiB @@ -217,7 +221,9 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # Enabling this will result in the VM device having iommu_platform=on set #enable_iommu_platform = true -# List of valid annotations values for the vhost user store path (default: empty) +# List of valid annotations values for the vhost user store path +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @DEFVHOSTUSERSTOREPATHLIST@ vhost_user_store_path_list = @DEFVHOSTUSERSTOREPATHLIST@ # Enable file based guest memory support. The default is an empty string which @@ -226,8 +232,10 @@ vhost_user_store_path_list = @DEFVHOSTUSERSTOREPATHLIST@ # This option will be ignored if VM templating is enabled. #file_mem_backend = "@DEFFILEMEMBACKEND@" -# List of valid annotations values for the file_mem_backend annotation (default: empty) -#file_mem_backend_list = @DEFFILEMEMBACKENDLIST@ +# List of valid annotations values for the file_mem_backend annotation +# The default if not set is empty (all annotations rejected.) +# Your distribution recommends: @DEFFILEMEMBACKENDLIST@ +file_mem_backend_list = @DEFFILEMEMBACKENDLIST@ # Enable swap of vm memory. Default false. # The behaviour is undefined if mem_prealloc is also set to true From b5db114aad195d077794776f1c32dc11d9892007 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 10 Sep 2020 19:47:41 +0200 Subject: [PATCH 17/24] annotations: Rename checkPathIsInGlobList with checkPathIsInGlobs The name is shorter and more specific Fixes: #901 Suggested-by: James O.D. Hunt Signed-off-by: Christophe de Dinechin --- src/runtime/virtcontainers/pkg/oci/utils.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index 5d02f5a378..adb2d8acd5 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -201,8 +201,8 @@ func regexpContains(s []string, e string) bool { return false } -func checkPathIsInGlobList(list []string, path string) bool { - for _, glob := range list { +func checkPathIsInGlobs(globs []string, path string) bool { + for _, glob := range globs { filenames, _ := filepath.Glob(glob) for _, a := range filenames { if path == a { @@ -417,21 +417,21 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, } if value, ok := ocispec.Annotations[vcAnnotations.HypervisorPath]; ok { - if !checkPathIsInGlobList(runtime.HypervisorConfig.HypervisorPathList, value) { + if !checkPathIsInGlobs(runtime.HypervisorConfig.HypervisorPathList, value) { return fmt.Errorf("hypervisor %v required from annotation is not valid", value) } config.HypervisorConfig.HypervisorPath = value } if value, ok := ocispec.Annotations[vcAnnotations.JailerPath]; ok { - if !checkPathIsInGlobList(runtime.HypervisorConfig.JailerPathList, value) { + if !checkPathIsInGlobs(runtime.HypervisorConfig.JailerPathList, value) { return fmt.Errorf("jailer %v required from annotation is not valid", value) } config.HypervisorConfig.JailerPath = value } if value, ok := ocispec.Annotations[vcAnnotations.CtlPath]; ok { - if !checkPathIsInGlobList(runtime.HypervisorConfig.HypervisorCtlPathList, value) { + if !checkPathIsInGlobs(runtime.HypervisorConfig.HypervisorCtlPathList, value) { return fmt.Errorf("hypervisor control %v required from annotation is not valid", value) } config.HypervisorConfig.HypervisorCtlPath = value @@ -461,7 +461,7 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, } if value, ok := ocispec.Annotations[vcAnnotations.VhostUserStorePath]; ok { - if !checkPathIsInGlobList(runtime.HypervisorConfig.VhostUserStorePathList, value) { + if !checkPathIsInGlobs(runtime.HypervisorConfig.VhostUserStorePathList, value) { return fmt.Errorf("vhost store path %v required from annotation is not valid", value) } config.HypervisorConfig.VhostUserStorePath = value @@ -586,7 +586,7 @@ func addHypervisorMemoryOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig } if value, ok := ocispec.Annotations[vcAnnotations.FileBackedMemRootDir]; ok { - if !checkPathIsInGlobList(runtime.HypervisorConfig.FileBackedMemRootList, value) { + if !checkPathIsInGlobs(runtime.HypervisorConfig.FileBackedMemRootList, value) { return fmt.Errorf("file_mem_backend value %v required from annotation is not valid", value) } sbConfig.HypervisorConfig.FileBackedMemRootDir = value @@ -742,7 +742,7 @@ func addHypervisorVirtioFsOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConf } if value, ok := ocispec.Annotations[vcAnnotations.VirtioFSDaemon]; ok { - if !checkPathIsInGlobList(runtime.HypervisorConfig.VirtioFSDaemonList, value) { + if !checkPathIsInGlobs(runtime.HypervisorConfig.VirtioFSDaemonList, value) { return fmt.Errorf("virtiofs daemon %v required from annotation is not valid", value) } sbConfig.HypervisorConfig.VirtioFSDaemon = value From b119427405b761e35a84ecc99a2f92d56148e67d Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 10 Sep 2020 19:54:12 +0200 Subject: [PATCH 18/24] annotations: Give better names to local variabes in search functions Use more meaningful variable names for clarity. Fixes: #901 Suggested-by: James O.D. Hunt james.o.hunt@intel.com> Signed-off-by: Christophe de Dinechin --- src/runtime/virtcontainers/pkg/oci/utils.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index adb2d8acd5..6e3e272176 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -183,18 +183,18 @@ func containerMounts(spec specs.Spec) []vc.Mount { return mnts } -func contains(s []string, e string) bool { - for _, a := range s { - if a == e { +func contains(strings []string, toFind string) bool { + for _, candidate := range strings { + if candidate == toFind { return true } } return false } -func regexpContains(s []string, e string) bool { - for _, a := range s { - if matched, _ := regexp.MatchString(a, e); matched { +func regexpContains(regexps []string, toMatch string) bool { + for _, candidate := range regexps { + if matched, _ := regexp.MatchString(candidate, toMatch); matched { return true } } From be6ee2550d5387736a7af1247a82d3d6b7a77e06 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 10 Sep 2020 21:22:59 +0200 Subject: [PATCH 19/24] makefile: Improve names of config entries for annotation checks The entries used to be things like PATH_LIST, which are too generic. Replace them with more precise name with a distinguishing keyword, namely VALID. For example valid_hypervisor_paths. Fixes: #901 Suggested-by: James O.D. Hunt Signed-off-by: Christophe de Dinechin --- src/runtime/Makefile | 58 +++++++++---------- .../cli/config/configuration-acrn.toml.in | 8 +-- .../cli/config/configuration-clh.toml.in | 8 +-- .../cli/config/configuration-fc.toml.in | 8 +-- .../configuration-qemu-virtiofs.toml.in | 16 ++--- .../cli/config/configuration-qemu.toml.in | 16 ++--- src/runtime/pkg/katautils/config.go | 12 ++-- 7 files changed, 63 insertions(+), 63 deletions(-) diff --git a/src/runtime/Makefile b/src/runtime/Makefile index 784c4c705c..8d657e7643 100644 --- a/src/runtime/Makefile +++ b/src/runtime/Makefile @@ -129,22 +129,22 @@ DEFAULT_HYPERVISOR ?= $(HYPERVISOR_QEMU) HYPERVISORS := $(HYPERVISOR_ACRN) $(HYPERVISOR_FC) $(HYPERVISOR_QEMU) $(HYPERVISOR_QEMU_VIRTIOFS) $(HYPERVISOR_CLH) QEMUPATH := $(QEMUBINDIR)/$(QEMUCMD) -QEMUPATHLIST := [\"$(QEMUPATH)\"] +QEMUVALIDHYPERVISORPATHS := [\"$(QEMUPATH)\"] -QEMUVIRTIOFSPATH := $(QEMUBINDIR)/$(QEMUVIRTIOFSCMD) +QEMUVALIDVIRTIOFSPATHS := $(QEMUBINDIR)/$(QEMUVIRTIOFSCMD) CLHPATH := $(CLHBINDIR)/$(CLHCMD) -CLHPATHLIST := [\"$(CLHBINDIR)/$(CLHCMD)\"] +CLHVALIDHYPERVISORPATHS := [\"$(CLHBINDIR)/$(CLHCMD)\"] FCPATH = $(FCBINDIR)/$(FCCMD) -FCPATHLIST = [\"$(FCPATH)\"] +FCVALIDPATHS = [\"$(FCPATH)\"] FCJAILERPATH = $(FCBINDIR)/$(FCJAILERCMD) -FCJAILERPATHLIST = [\"$(FCJAILERPATH)\"] +FCVALIDJAILERPATHS = [\"$(FCJAILERPATH)\"] ACRNPATH := $(ACRNBINDIR)/$(ACRNCMD) -ACRNPATHLIST := [\"$(ACRNPATH)\"] +ACRNVALIDHYPERVISORPATHS := [\"$(ACRNPATH)\"] ACRNCTLPATH := $(ACRNBINDIR)/$(ACRNCTLCMD) -ACRNCTLPATHLIST := [\"$(ACRNCTLPATH)\"] +ACRNVALIDCTLPATHS := [\"$(ACRNCTLPATH)\"] SHIMCMD := $(BIN_PREFIX)-shim SHIMPATH := $(PKGLIBEXECDIR)/$(SHIMCMD) @@ -179,7 +179,7 @@ DEFDISABLEBLOCK := false DEFSHAREDFS := virtio-9p DEFSHAREDFS_QEMU_VIRTIOFS := virtio-fs DEFVIRTIOFSDAEMON := $(VIRTIOFSDBINDIR)/virtiofsd -DEFVIRTIOFSDAEMONLIST := [\"$(DEFVIRTIOFSDAEMON)\"] +DEFVALIDVIRTIOFSDAEMONPATHS := [\"$(DEFVIRTIOFSDAEMON)\"] # Default DAX mapping cache size in MiB #if value is 0, DAX is not enabled DEFVIRTIOFSCACHESIZE := 0 @@ -195,9 +195,9 @@ DEFENABLEMEMPREALLOC := false DEFENABLEHUGEPAGES := false DEFENABLEVHOSTUSERSTORE := false DEFVHOSTUSERSTOREPATH := $(PKGRUNDIR)/vhost-user -DEFVHOSTUSERSTOREPATHLIST := [\"$(DEFVHOSTUSERSTOREPATH)\"] +DEFVALIDVHOSTUSERSTOREPATHS := [\"$(DEFVHOSTUSERSTOREPATH)\"] DEFFILEMEMBACKEND := "" -DEFFILEMEMBACKENDLIST := [\"$(DEFFILEMEMBACKEND)\"] +DEFVALIDFILEMEMBACKENDS := [\"$(DEFFILEMEMBACKEND)\"] DEFENABLESWAP := false DEFENABLEDEBUG := false DEFDISABLENESTINGCHECKS := false @@ -404,14 +404,14 @@ USER_VARS += ACRNCTLCMD USER_VARS += ACRNPATH USER_VARS += ACRNPATHLIST USER_VARS += ACRNCTLPATH -USER_VARS += ACRNCTLPATHLIST +USER_VARS += ACRNVALIDCTLPATHS USER_VARS += CLHPATH -USER_VARS += CLHPATHLIST +USER_VARS += CLHVALIDHYPERVISORPATHS USER_VARS += FCCMD USER_VARS += FCPATH -USER_VARS += FCPATHLIST +USER_VARS += FCVALIDHYPERVISORPATHS USER_VARS += FCJAILERPATH -USER_VARS += FCJAILERPATHLIST +USER_VARS += FCVALIDJAILERPATHS USER_VARS += SYSCONFIG USER_VARS += IMAGENAME USER_VARS += IMAGEPATH @@ -442,10 +442,10 @@ USER_VARS += NETMONPATH USER_VARS += QEMUBINDIR USER_VARS += QEMUCMD USER_VARS += QEMUPATH -USER_VARS += QEMUPATHLIST +USER_VARS += QEMUVALIDPATHS USER_VARS += QEMUVIRTIOFSCMD USER_VARS += QEMUVIRTIOFSPATH -USER_VARS += QEMUVIRTIOFSPATHLIST +USER_VARS += QEMUVALIDVIRTIOFSPATHS USER_VARS += SHAREDIR USER_VARS += SHIMPATH USER_VARS += SYSCONFDIR @@ -468,7 +468,7 @@ USER_VARS += DEFBLOCKSTORAGEDRIVER_QEMU_VIRTIOFS USER_VARS += DEFSHAREDFS USER_VARS += DEFSHAREDFS_QEMU_VIRTIOFS USER_VARS += DEFVIRTIOFSDAEMON -USER_VARS += DEFVIRTIOFSDAEMONLIST +USER_VARS += DEFVALIDVIRTIOFSDAEMONPATHS USER_VARS += DEFVIRTIOFSCACHESIZE USER_VARS += DEFVIRTIOFSCACHE USER_VARS += DEFVIRTIOFSEXTRAARGS @@ -477,9 +477,9 @@ USER_VARS += DEFENABLEMEMPREALLOC USER_VARS += DEFENABLEHUGEPAGES USER_VARS += DEFENABLEVHOSTUSERSTORE USER_VARS += DEFVHOSTUSERSTOREPATH -USER_VARS += DEFVHOSTUSERSTOREPATHLIST +USER_VARS += DEFVALIDVHOSTUSERSTOREPATHS USER_VARS += DEFFILEMEMBACKEND -USER_VARS += DEFFILEMEMBACKENDLIST +USER_VARS += DEFVALIDFILEMEMBACKENDS USER_VARS += DEFENABLESWAP USER_VARS += DEFENABLEDEBUG USER_VARS += DEFDISABLENESTINGCHECKS @@ -629,15 +629,15 @@ $(GENERATED_FILES): %: %.in $(MAKEFILE_LIST) VERSION .git-commit -e "s|@CONFIG_FC_IN@|$(CONFIG_FC_IN)|g" \ -e "s|@CONFIG_PATH@|$(CONFIG_PATH)|g" \ -e "s|@FCPATH@|$(FCPATH)|g" \ - -e "s|@FCPATHLIST@|$(FCPATHLIST)|g" \ + -e "s|@FCVALIDHYPERVISORPATHS@|$(FCVALIDHYPERVISORPATHS)|g" \ -e "s|@FCJAILERPATH@|$(FCJAILERPATH)|g" \ - -e "s|@FCJAILERPATHLIST@|$(FCJAILERPATHLIST)|g" \ + -e "s|@FCVALIDJAILERPATHS@|$(FCVALIDJAILERPATHS)|g" \ -e "s|@ACRNPATH@|$(ACRNPATH)|g" \ - -e "s|@ACRNPATHLIST@|$(ACRNPATHLIST)|g" \ + -e "s|@ACRNVALIDHYPERVISORPATHS@|$(ACRNVALIDHYPERVISORPATHS)|g" \ -e "s|@ACRNCTLPATH@|$(ACRNCTLPATH)|g" \ - -e "s|@ACRNCTLPATHLIST@|$(ACRNCTLPATHLIST)|g" \ + -e "s|@ACRNVALIDCTLPATHS@|$(ACRNVALIDCTLPATHS)|g" \ -e "s|@CLHPATH@|$(CLHPATH)|g" \ - -e "s|@CLHPATHLIST@|$(CLHPATHLIST)|g" \ + -e "s|@CLHVALIDHYPERVISORPATHS@|$(CLHVALIDHYPERVISORPATHS)|g" \ -e "s|@SYSCONFIG@|$(SYSCONFIG)|g" \ -e "s|@IMAGEPATH@|$(IMAGEPATH)|g" \ -e "s|@KERNELPATH_ACRN@|$(KERNELPATH_ACRN)|g" \ @@ -663,9 +663,9 @@ $(GENERATED_FILES): %: %.in $(MAKEFILE_LIST) VERSION .git-commit -e "s|@PROJECT_TAG@|$(PROJECT_TAG)|g" \ -e "s|@PROJECT_TYPE@|$(PROJECT_TYPE)|g" \ -e "s|@QEMUPATH@|$(QEMUPATH)|g" \ - -e "s|@QEMUPATHLIST@|$(QEMUPATHLIST)|g" \ + -e "s|@QEMUVALIDHYPERVISORPATHS@|$(QEMUVALIDHYPERVISORPATHS)|g" \ -e "s|@QEMUVIRTIOFSPATH@|$(QEMUVIRTIOFSPATH)|g" \ - -e "s|@QEMUVIRTIOFSPATHLIST@|$(QEMUVIRTIOFSPATHLIST)|g" \ + -e "s|@QEMUVALIDVIRTIOFSPATHS@|$(QEMUVALIDVIRTIOFSPATHS)|g" \ -e "s|@RUNTIME_NAME@|$(TARGET)|g" \ -e "s|@MACHINETYPE@|$(MACHINETYPE)|g" \ -e "s|@SHIMPATH@|$(SHIMPATH)|g" \ @@ -690,7 +690,7 @@ $(GENERATED_FILES): %: %.in $(MAKEFILE_LIST) VERSION .git-commit -e "s|@DEFSHAREDFS@|$(DEFSHAREDFS)|g" \ -e "s|@DEFSHAREDFS_QEMU_VIRTIOFS@|$(DEFSHAREDFS_QEMU_VIRTIOFS)|g" \ -e "s|@DEFVIRTIOFSDAEMON@|$(DEFVIRTIOFSDAEMON)|g" \ - -e "s|@DEFVIRTIOFSDAEMONLIST@|$(DEFVIRTIOFSDAEMONLIST)|g" \ + -e "s|@DEFVALIDVIRTIOFSDAEMONPATHS@|$(DEFVALIDVIRTIOFSDAEMONPATHS)|g" \ -e "s|@DEFVIRTIOFSCACHESIZE@|$(DEFVIRTIOFSCACHESIZE)|g" \ -e "s|@DEFVIRTIOFSCACHE@|$(DEFVIRTIOFSCACHE)|g" \ -e "s|@DEFVIRTIOFSEXTRAARGS@|$(DEFVIRTIOFSEXTRAARGS)|g" \ @@ -699,9 +699,9 @@ $(GENERATED_FILES): %: %.in $(MAKEFILE_LIST) VERSION .git-commit -e "s|@DEFENABLEHUGEPAGES@|$(DEFENABLEHUGEPAGES)|g" \ -e "s|@DEFENABLEVHOSTUSERSTORE@|$(DEFENABLEVHOSTUSERSTORE)|g" \ -e "s|@DEFVHOSTUSERSTOREPATH@|$(DEFVHOSTUSERSTOREPATH)|g" \ - -e "s|@DEFVHOSTUSERSTOREPATHLIST@|$(DEFVHOSTUSERSTOREPATHLIST)|g" \ + -e "s|@DEFVALIDVHOSTUSERSTOREPATHS@|$(DEFVALIDVHOSTUSERSTOREPATHS)|g" \ -e "s|@DEFFILEMEMBACKEND@|$(DEFFILEMEMBACKEND)|g" \ - -e "s|@DEFFILEMEMBACKENDLIST@|$(DEFFILEMEMBACKENDLIST)|g" \ + -e "s|@DEFVALIDFILEMEMBACKENDS@|$(DEFVALIDFILEMEMBACKENDS)|g" \ -e "s|@DEFENABLEMSWAP@|$(DEFENABLESWAP)|g" \ -e "s|@DEFENABLEDEBUG@|$(DEFENABLEDEBUG)|g" \ -e "s|@DEFDISABLENESTINGCHECKS@|$(DEFDISABLENESTINGCHECKS)|g" \ diff --git a/src/runtime/cli/config/configuration-acrn.toml.in b/src/runtime/cli/config/configuration-acrn.toml.in index 85d60cd443..6ffa8d5d7a 100644 --- a/src/runtime/cli/config/configuration-acrn.toml.in +++ b/src/runtime/cli/config/configuration-acrn.toml.in @@ -24,13 +24,13 @@ enable_annotations = @DEFENABLEANNOTATIONS@ # List of valid annotations values for the hypervisor # Each member of the list is a path pattern as described by glob(3). # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @ACRNPATHLIST@ -path_list = @ACRNPATHLIST@ +# Your distribution recommends: @ACRNVALIDHYPERVISORPATHS@ +valid_hypervisor_paths = @ACRNVALIDHYPERVISORPATHS@ # List of valid annotations values for ctlpath # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @ACRNCTLPATHLIST@ -ctlpath_list = @ACRNCTLPATHLIST@ +# Your distribution recommends: @ACRNVALIDCTLPATHS@ +valid_ctlpaths = @ACRNVALIDCTLPATHS@ # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having diff --git a/src/runtime/cli/config/configuration-clh.toml.in b/src/runtime/cli/config/configuration-clh.toml.in index 33379c8852..4cc528d564 100644 --- a/src/runtime/cli/config/configuration-clh.toml.in +++ b/src/runtime/cli/config/configuration-clh.toml.in @@ -23,8 +23,8 @@ enable_annotations = @DEFENABLEANNOTATIONS@ # List of valid annotations values for the hypervisor # Each member of the list is a path pattern as described by glob(3). # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @CLHPATHLIST@ -path_list = @CLHPATHLIST@ +# Your distribution recommends: @CLHVALIDHYPERVISORPATHS@ +valid_hypervisor_paths = @CLHVALIDHYPERVISORPATHS@ # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having @@ -75,8 +75,8 @@ virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" # List of valid annotations values for the virtiofs daemon # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @DEFVIRTIOFSDAEMONLIST@ -virtio_fs_daemon_list = @DEFVIRTIOFSDAEMONLIST@ +# Your distribution recommends: @DEFVALIDVIRTIOFSDAEMONPATHS@ +valid_virtio_fs_daemon_paths = @DEFVALIDVIRTIOFSDAEMONPATHS@ # Default size of DAX cache in MiB virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ diff --git a/src/runtime/cli/config/configuration-fc.toml.in b/src/runtime/cli/config/configuration-fc.toml.in index e21d2c1ddc..e6a2b823b7 100644 --- a/src/runtime/cli/config/configuration-fc.toml.in +++ b/src/runtime/cli/config/configuration-fc.toml.in @@ -23,8 +23,8 @@ enable_annotations = @DEFENABLEANNOTATIONS@ # List of valid annotations values for the hypervisor # Each member of the list is a path pattern as described by glob(3). # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @FCPATHLIST@ -path_list = @FCPATHLIST@ +# Your distribution recommends: @FCVALIDHYPERVISORPATHS@ +valid_hypervisor_paths = @FCVALIDHYPERVISORPATHS@ # Path for the jailer specific to firecracker # If the jailer path is not set kata will launch firecracker @@ -37,8 +37,8 @@ path_list = @FCPATHLIST@ # List of valid jailer path values for the hypervisor # Each member of the list can be a regular expression # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @FCJAILERPATHLIST@ -jailer_path_list = @FCJAILERPATHLIST@ +# Your distribution recommends: @FCVALIDJAILERPATHS@ +valid_jailer_paths = @FCVALIDJAILERPATHS@ # Optional space-separated list of options to pass to the guest kernel. diff --git a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in index 27c963d749..41186907dd 100644 --- a/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in +++ b/src/runtime/cli/config/configuration-qemu-virtiofs.toml.in @@ -26,8 +26,8 @@ enable_annotations = @DEFENABLEANNOTATIONS@ # List of valid annotations values for the hypervisor # Each member of the list is a path pattern as described by glob(3). # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @QEMUVIRTIOFSPATHLIST@ -path_list = @QEMUVIRTIOFSPATHLIST@ +# Your distribution recommends: @QEMUVALIDHYPERVISORPATHS@ +valid_hypervisor_paths = @QEMUVALIDHYPERVISORPATHS@ # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having @@ -125,8 +125,8 @@ virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" # List of valid annotations values for the virtiofs daemon # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @DEFVIRTIOFSDAEMONLIST@ -virtio_fs_daemon_list = @DEFVIRTIOFSDAEMONLIST@ +# Your distribution recommends: @DEFVALIDVIRTIOFSDAEMONPATHS@ +valid_virtio_fs_daemon_paths = @DEFVALIDVIRTIOFSDAEMONPATHS@ # Default size of DAX cache in MiB virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ @@ -219,8 +219,8 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # List of valid annotations values for the virtiofs daemon # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @DEFVHOSTUSERSTOREPATHLIST@ -vhost_user_store_path_list = @DEFVHOSTUSERSTOREPATHLIST@ +# Your distribution recommends: @DEFVALIDVHOSTUSERSTOREPATHS@ +valid_vhost_user_store_paths = @DEFVALIDVHOSTUSERSTOREPATHS@ # Enable file based guest memory support. The default is an empty string which # will disable this feature. In the case of virtio-fs, this is enabled @@ -230,8 +230,8 @@ vhost_user_store_path_list = @DEFVHOSTUSERSTOREPATHLIST@ # List of valid annotations values for the file_mem_backend annotation # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @DEFFILEMEMBACKENDLIST@ -file_mem_backend_list = @DEFFILEMEMBACKENDLIST@ +# Your distribution recommends: @DEFVALIDVHOSTUSERSTOREPATHS@ +valid_file_mem_backends = @DEFVALIDFILEMEMBACKENDS@ # Enable swap of vm memory. Default false. # The behaviour is undefined if mem_prealloc is also set to true diff --git a/src/runtime/cli/config/configuration-qemu.toml.in b/src/runtime/cli/config/configuration-qemu.toml.in index 87b10d3175..0e6051430a 100644 --- a/src/runtime/cli/config/configuration-qemu.toml.in +++ b/src/runtime/cli/config/configuration-qemu.toml.in @@ -24,8 +24,8 @@ enable_annotations = @DEFENABLEANNOTATIONS@ # List of valid annotations values for the hypervisor # Each member of the list is a path pattern as described by glob(3). # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @QEMUPATHLIST@ -path_list = @QEMUPATHLIST@ +# Your distribution recommends: @QEMUVALIDHYPERVISORPATHS@ +valid_hypervisor_paths = @QEMUVALIDHYPERVISORPATHS@ # Optional space-separated list of options to pass to the guest kernel. # For example, use `kernel_params = "vsyscall=emulate"` if you are having @@ -129,8 +129,8 @@ virtio_fs_daemon = "@DEFVIRTIOFSDAEMON@" # List of valid annotations values for the virtiofs daemon # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @DEFVIRTIOFSDAEMONLIST@ -virtio_fs_daemon_list = @DEFVIRTIOFSDAEMONLIST@ +# Your distribution recommends: @DEFVALIDVIRTIOFSDAEMONPATHS@ +valid_virtio_fs_daemon_paths = @DEFVALIDVIRTIOFSDAEMONPATHS@ # Default size of DAX cache in MiB virtio_fs_cache_size = @DEFVIRTIOFSCACHESIZE@ @@ -223,8 +223,8 @@ vhost_user_store_path = "@DEFVHOSTUSERSTOREPATH@" # List of valid annotations values for the vhost user store path # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @DEFVHOSTUSERSTOREPATHLIST@ -vhost_user_store_path_list = @DEFVHOSTUSERSTOREPATHLIST@ +# Your distribution recommends: @DEFVALIDVHOSTUSERSTOREPATHS@ +valid_vhost_user_store_paths = @DEFVALIDVHOSTUSERSTOREPATHS@ # Enable file based guest memory support. The default is an empty string which # will disable this feature. In the case of virtio-fs, this is enabled @@ -234,8 +234,8 @@ vhost_user_store_path_list = @DEFVHOSTUSERSTOREPATHLIST@ # List of valid annotations values for the file_mem_backend annotation # The default if not set is empty (all annotations rejected.) -# Your distribution recommends: @DEFFILEMEMBACKENDLIST@ -file_mem_backend_list = @DEFFILEMEMBACKENDLIST@ +# Your distribution recommends: @DEFVALIDFILEMEMBACKENDS@ +valid_file_mem_backends = @DEFVALIDFILEMEMBACKENDS@ # Enable swap of vm memory. Default false. # The behaviour is undefined if mem_prealloc is also set to true diff --git a/src/runtime/pkg/katautils/config.go b/src/runtime/pkg/katautils/config.go index 12cf42438d..7ba2281ab9 100644 --- a/src/runtime/pkg/katautils/config.go +++ b/src/runtime/pkg/katautils/config.go @@ -71,12 +71,12 @@ type factory struct { type hypervisor struct { Path string `toml:"path"` - HypervisorPathList []string `toml:"path_list"` + HypervisorPathList []string `toml:"valid_hypervisor_paths"` JailerPath string `toml:"jailer_path"` - JailerPathList []string `toml:"jailer_path_list"` + JailerPathList []string `toml:"valid_jailer_paths"` Kernel string `toml:"kernel"` CtlPath string `toml:"ctlpath"` - CtlPathList []string `toml:"ctlpath_list"` + CtlPathList []string `toml:"valid_ctlpaths"` Initrd string `toml:"initrd"` Image string `toml:"image"` Firmware string `toml:"firmware"` @@ -88,7 +88,7 @@ type hypervisor struct { EntropySource string `toml:"entropy_source"` SharedFS string `toml:"shared_fs"` VirtioFSDaemon string `toml:"virtio_fs_daemon"` - VirtioFSDaemonList []string `toml:"virtio_fs_daemon_list"` + VirtioFSDaemonList []string `toml:"valid_virtio_fs_daemon_paths"` VirtioFSCache string `toml:"virtio_fs_cache"` VirtioFSExtraArgs []string `toml:"virtio_fs_extra_args"` VirtioFSCacheSize uint32 `toml:"virtio_fs_cache_size"` @@ -97,7 +97,7 @@ type hypervisor struct { BlockDeviceCacheNoflush bool `toml:"block_device_cache_noflush"` EnableVhostUserStore bool `toml:"enable_vhost_user_store"` VhostUserStorePath string `toml:"vhost_user_store_path"` - VhostUserStorePathList []string `toml:"vhost_user_store_path_list"` + VhostUserStorePathList []string `toml:"valid_vhost_user_store_paths"` NumVCPUs int32 `toml:"default_vcpus"` DefaultMaxVCPUs uint32 `toml:"default_maxvcpus"` MemorySize uint32 `toml:"default_memory"` @@ -113,7 +113,7 @@ type hypervisor struct { IOMMU bool `toml:"enable_iommu"` IOMMUPlatform bool `toml:"enable_iommu_platform"` FileBackedMemRootDir string `toml:"file_mem_backend"` - FileBackedMemRootList []string `toml:"file_mem_backend_list"` + FileBackedMemRootList []string `toml:"valid_file_mem_backends"` Swap bool `toml:"enable_swap"` Debug bool `toml:"enable_debug"` DisableNestingChecks bool `toml:"disable_nesting_checks"` From 966bd573442d2f0b951e21aae2445dd009b3d5d1 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 10 Sep 2020 21:50:24 +0200 Subject: [PATCH 20/24] makefile: Add missing generated vars to `USER_VARS` This was discovered while checking a massive change in variables. The root cause for the error is a very long list of manual replacements, that is best replaced with a $(foreach). All individual variables in the output configuration files were checked against the old build using diff. This is a forward port of a makefile fix included in PR https://github.com/kata-containers/runtime/issues/3004 for issue https://github.com/kata-containers/runtime/issues/2943 Fixes: #901 Signed-off-by: Christophe de Dinechin --- src/runtime/Makefile | 122 +++++++++++-------------------------------- 1 file changed, 30 insertions(+), 92 deletions(-) diff --git a/src/runtime/Makefile b/src/runtime/Makefile index 8d657e7643..a5b8d4ef6f 100644 --- a/src/runtime/Makefile +++ b/src/runtime/Makefile @@ -95,6 +95,14 @@ COLLECT_SCRIPT = data/kata-collect-data.sh COLLECT_SCRIPT_SRC = $(COLLECT_SCRIPT).in GENERATED_FILES += $(COLLECT_SCRIPT) +GENERATED_VARS = \ + VERSION \ + CONFIG_ACRN_IN \ + CONFIG_QEMU_IN \ + CONFIG_QEMU_VIRTIOFS_IN \ + CONFIG_CLH_IN \ + CONFIG_FC_IN \ + $(USER_VARS) SCRIPTS += $(COLLECT_SCRIPT) SCRIPTS_DIR := $(BINDIR) @@ -396,17 +404,24 @@ SHAREDIR := $(SHAREDIR) # list of variables the user may wish to override USER_VARS += ARCH USER_VARS += BINDIR +USER_VARS += CONFIG_ACRN_IN +USER_VARS += CONFIG_CLH_IN +USER_VARS += CONFIG_FC_IN USER_VARS += CONFIG_PATH +USER_VARS += CONFIG_QEMU_IN +USER_VARS += CONFIG_QEMU_VIRTIOFS_IN USER_VARS += DESTDIR USER_VARS += DEFAULT_HYPERVISOR +USER_VARS += DEFENABLEMSWAP USER_VARS += ACRNCMD USER_VARS += ACRNCTLCMD USER_VARS += ACRNPATH -USER_VARS += ACRNPATHLIST +USER_VARS += ACRNVALIDHYPERVISORPATHS USER_VARS += ACRNCTLPATH USER_VARS += ACRNVALIDCTLPATHS USER_VARS += CLHPATH USER_VARS += CLHVALIDHYPERVISORPATHS +USER_VARS += FIRMWAREPATH_CLH USER_VARS += FCCMD USER_VARS += FCPATH USER_VARS += FCVALIDHYPERVISORPATHS @@ -423,6 +438,11 @@ USER_VARS += KERNELTYPE USER_VARS += KERNELTYPE_FC USER_VARS += KERNELTYPE_ACRN USER_VARS += KERNELTYPE_CLH +USER_VARS += KERNELPATH_ACRN +USER_VARS += KERNELPATH +USER_VARS += KERNELPATH_CLH +USER_VARS += KERNELPATH_FC +USER_VARS += KERNELVIRTIOFSPATH USER_VARS += FIRMWAREPATH USER_VARS += MACHINEACCELERATORS USER_VARS += CPUFEATURES @@ -435,17 +455,22 @@ USER_VARS += PKGLIBDIR USER_VARS += PKGLIBEXECDIR USER_VARS += PKGRUNDIR USER_VARS += PREFIX +USER_VARS += PROJECT_BUG_URL USER_VARS += PROJECT_NAME +USER_VARS += PROJECT_ORG USER_VARS += PROJECT_PREFIX +USER_VARS += PROJECT_TAG USER_VARS += PROJECT_TYPE +USER_VARS += PROJECT_URL USER_VARS += NETMONPATH USER_VARS += QEMUBINDIR USER_VARS += QEMUCMD USER_VARS += QEMUPATH -USER_VARS += QEMUVALIDPATHS +USER_VARS += QEMUVALIDHYPERVISORPATHS USER_VARS += QEMUVIRTIOFSCMD USER_VARS += QEMUVIRTIOFSPATH USER_VARS += QEMUVALIDVIRTIOFSPATHS +USER_VARS += RUNTIME_NAME USER_VARS += SHAREDIR USER_VARS += SHIMPATH USER_VARS += SYSCONFDIR @@ -456,6 +481,7 @@ USER_VARS += DEFMEMSZ USER_VARS += DEFMEMSLOTS USER_VARS += DEFBRIDGES USER_VARS += DEFNETWORKMODEL_ACRN +USER_VARS += DEFNETWORKMODEL_CLH USER_VARS += DEFNETWORKMODEL_FC USER_VARS += DEFNETWORKMODEL_QEMU USER_VARS += DEFDISABLEGUESTSECCOMP @@ -472,6 +498,7 @@ USER_VARS += DEFVALIDVIRTIOFSDAEMONPATHS USER_VARS += DEFVIRTIOFSCACHESIZE USER_VARS += DEFVIRTIOFSCACHE USER_VARS += DEFVIRTIOFSEXTRAARGS +USER_VARS += DEFENABLEANNOTATIONS USER_VARS += DEFENABLEIOTHREADS USER_VARS += DEFENABLEMEMPREALLOC USER_VARS += DEFENABLEHUGEPAGES @@ -621,96 +648,7 @@ GENERATED_FILES += $(CONFIGS) $(GENERATED_FILES): %: %.in $(MAKEFILE_LIST) VERSION .git-commit $(QUIET_GENERATE)$(SED) \ -e "s|@COMMIT@|$(shell cat .git-commit)|g" \ - -e "s|@VERSION@|$(VERSION)|g" \ - -e "s|@CONFIG_ACRN_IN@|$(CONFIG_ACRN_IN)|g" \ - -e "s|@CONFIG_QEMU_IN@|$(CONFIG_QEMU_IN)|g" \ - -e "s|@CONFIG_QEMU_VIRTIOFS_IN@|$(CONFIG_QEMU_VIRTIOFS_IN)|g" \ - -e "s|@CONFIG_CLH_IN@|$(CONFIG_CLH_IN)|g" \ - -e "s|@CONFIG_FC_IN@|$(CONFIG_FC_IN)|g" \ - -e "s|@CONFIG_PATH@|$(CONFIG_PATH)|g" \ - -e "s|@FCPATH@|$(FCPATH)|g" \ - -e "s|@FCVALIDHYPERVISORPATHS@|$(FCVALIDHYPERVISORPATHS)|g" \ - -e "s|@FCJAILERPATH@|$(FCJAILERPATH)|g" \ - -e "s|@FCVALIDJAILERPATHS@|$(FCVALIDJAILERPATHS)|g" \ - -e "s|@ACRNPATH@|$(ACRNPATH)|g" \ - -e "s|@ACRNVALIDHYPERVISORPATHS@|$(ACRNVALIDHYPERVISORPATHS)|g" \ - -e "s|@ACRNCTLPATH@|$(ACRNCTLPATH)|g" \ - -e "s|@ACRNVALIDCTLPATHS@|$(ACRNVALIDCTLPATHS)|g" \ - -e "s|@CLHPATH@|$(CLHPATH)|g" \ - -e "s|@CLHVALIDHYPERVISORPATHS@|$(CLHVALIDHYPERVISORPATHS)|g" \ - -e "s|@SYSCONFIG@|$(SYSCONFIG)|g" \ - -e "s|@IMAGEPATH@|$(IMAGEPATH)|g" \ - -e "s|@KERNELPATH_ACRN@|$(KERNELPATH_ACRN)|g" \ - -e "s|@KERNELPATH_FC@|$(KERNELPATH_FC)|g" \ - -e "s|@KERNELPATH_CLH@|$(KERNELPATH_CLH)|g" \ - -e "s|@KERNELPATH@|$(KERNELPATH)|g" \ - -e "s|@KERNELVIRTIOFSPATH@|$(KERNELVIRTIOFSPATH)|g" \ - -e "s|@INITRDPATH@|$(INITRDPATH)|g" \ - -e "s|@FIRMWAREPATH@|$(FIRMWAREPATH)|g" \ - -e "s|@MACHINEACCELERATORS@|$(MACHINEACCELERATORS)|g" \ - -e "s|@CPUFEATURES@|$(CPUFEATURES)|g" \ - -e "s|@FIRMWAREPATH_CLH@|$(FIRMWAREPATH_CLH)|g" \ - -e "s|@DEFMACHINETYPE_CLH@|$(DEFMACHINETYPE_CLH)|g" \ - -e "s|@KERNELPARAMS@|$(KERNELPARAMS)|g" \ - -e "s|@LOCALSTATEDIR@|$(LOCALSTATEDIR)|g" \ - -e "s|@PKGLIBEXECDIR@|$(PKGLIBEXECDIR)|g" \ - -e "s|@PKGRUNDIR@|$(PKGRUNDIR)|g" \ - -e "s|@NETMONPATH@|$(NETMONPATH)|g" \ - -e "s|@PROJECT_BUG_URL@|$(PROJECT_BUG_URL)|g" \ - -e "s|@PROJECT_ORG@|$(PROJECT_ORG)|g" \ - -e "s|@PROJECT_URL@|$(PROJECT_URL)|g" \ - -e "s|@PROJECT_NAME@|$(PROJECT_NAME)|g" \ - -e "s|@PROJECT_TAG@|$(PROJECT_TAG)|g" \ - -e "s|@PROJECT_TYPE@|$(PROJECT_TYPE)|g" \ - -e "s|@QEMUPATH@|$(QEMUPATH)|g" \ - -e "s|@QEMUVALIDHYPERVISORPATHS@|$(QEMUVALIDHYPERVISORPATHS)|g" \ - -e "s|@QEMUVIRTIOFSPATH@|$(QEMUVIRTIOFSPATH)|g" \ - -e "s|@QEMUVALIDVIRTIOFSPATHS@|$(QEMUVALIDVIRTIOFSPATHS)|g" \ - -e "s|@RUNTIME_NAME@|$(TARGET)|g" \ - -e "s|@MACHINETYPE@|$(MACHINETYPE)|g" \ - -e "s|@SHIMPATH@|$(SHIMPATH)|g" \ - -e "s|@DEFVCPUS@|$(DEFVCPUS)|g" \ - -e "s|@DEFMAXVCPUS@|$(DEFMAXVCPUS)|g" \ - -e "s|@DEFMAXVCPUS_ACRN@|$(DEFMAXVCPUS_ACRN)|g" \ - -e "s|@DEFMEMSZ@|$(DEFMEMSZ)|g" \ - -e "s|@DEFMEMSLOTS@|$(DEFMEMSLOTS)|g" \ - -e "s|@DEFBRIDGES@|$(DEFBRIDGES)|g" \ - -e "s|@DEFNETWORKMODEL_ACRN@|$(DEFNETWORKMODEL_ACRN)|g" \ - -e "s|@DEFNETWORKMODEL_CLH@|$(DEFNETWORKMODEL_CLH)|g" \ - -e "s|@DEFNETWORKMODEL_FC@|$(DEFNETWORKMODEL_FC)|g" \ - -e "s|@DEFNETWORKMODEL_QEMU@|$(DEFNETWORKMODEL_QEMU)|g" \ - -e "s|@DEFENABLEANNOTATIONS@|$(DEFENABLEANNOTATIONS)|g" \ - -e "s|@DEFDISABLEGUESTSECCOMP@|$(DEFDISABLEGUESTSECCOMP)|g" \ - -e "s|@DEFAULTEXPFEATURES@|$(DEFAULTEXPFEATURES)|g" \ - -e "s|@DEFDISABLEBLOCK@|$(DEFDISABLEBLOCK)|g" \ - -e "s|@DEFBLOCKSTORAGEDRIVER_ACRN@|$(DEFBLOCKSTORAGEDRIVER_ACRN)|g" \ - -e "s|@DEFBLOCKSTORAGEDRIVER_FC@|$(DEFBLOCKSTORAGEDRIVER_FC)|g" \ - -e "s|@DEFBLOCKSTORAGEDRIVER_QEMU@|$(DEFBLOCKSTORAGEDRIVER_QEMU)|g" \ - -e "s|@DEFBLOCKSTORAGEDRIVER_QEMU_VIRTIOFS@|$(DEFBLOCKSTORAGEDRIVER_QEMU_VIRTIOFS)|g" \ - -e "s|@DEFSHAREDFS@|$(DEFSHAREDFS)|g" \ - -e "s|@DEFSHAREDFS_QEMU_VIRTIOFS@|$(DEFSHAREDFS_QEMU_VIRTIOFS)|g" \ - -e "s|@DEFVIRTIOFSDAEMON@|$(DEFVIRTIOFSDAEMON)|g" \ - -e "s|@DEFVALIDVIRTIOFSDAEMONPATHS@|$(DEFVALIDVIRTIOFSDAEMONPATHS)|g" \ - -e "s|@DEFVIRTIOFSCACHESIZE@|$(DEFVIRTIOFSCACHESIZE)|g" \ - -e "s|@DEFVIRTIOFSCACHE@|$(DEFVIRTIOFSCACHE)|g" \ - -e "s|@DEFVIRTIOFSEXTRAARGS@|$(DEFVIRTIOFSEXTRAARGS)|g" \ - -e "s|@DEFENABLEIOTHREADS@|$(DEFENABLEIOTHREADS)|g" \ - -e "s|@DEFENABLEMEMPREALLOC@|$(DEFENABLEMEMPREALLOC)|g" \ - -e "s|@DEFENABLEHUGEPAGES@|$(DEFENABLEHUGEPAGES)|g" \ - -e "s|@DEFENABLEVHOSTUSERSTORE@|$(DEFENABLEVHOSTUSERSTORE)|g" \ - -e "s|@DEFVHOSTUSERSTOREPATH@|$(DEFVHOSTUSERSTOREPATH)|g" \ - -e "s|@DEFVALIDVHOSTUSERSTOREPATHS@|$(DEFVALIDVHOSTUSERSTOREPATHS)|g" \ - -e "s|@DEFFILEMEMBACKEND@|$(DEFFILEMEMBACKEND)|g" \ - -e "s|@DEFVALIDFILEMEMBACKENDS@|$(DEFVALIDFILEMEMBACKENDS)|g" \ - -e "s|@DEFENABLEMSWAP@|$(DEFENABLESWAP)|g" \ - -e "s|@DEFENABLEDEBUG@|$(DEFENABLEDEBUG)|g" \ - -e "s|@DEFDISABLENESTINGCHECKS@|$(DEFDISABLENESTINGCHECKS)|g" \ - -e "s|@DEFMSIZE9P@|$(DEFMSIZE9P)|g" \ - -e "s|@DEFHOTPLUGVFIOONROOTBUS@|$(DEFHOTPLUGVFIOONROOTBUS)|g" \ - -e "s|@DEFPCIEROOTPORT@|$(DEFPCIEROOTPORT)|g" \ - -e "s|@DEFENTROPYSOURCE@|$(DEFENTROPYSOURCE)|g" \ - -e "s|@DEFSANDBOXCGROUPONLY@|$(DEFSANDBOXCGROUPONLY)|g" \ - -e "s|@FEATURE_SELINUX@|$(FEATURE_SELINUX)|g" \ + $(foreach v,$(GENERATED_VARS),-e "s|@$v@|$($v)|g") \ $< > $@ generate-config: $(CONFIGS) From 6f52179ce490a1e65bc72a5fbbf1d19942c84ccb Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 11 Sep 2020 11:06:26 +0200 Subject: [PATCH 21/24] annotations: Add unit test for regexpContains function James O.D Hunt: "But also, regexpContains() and checkPathIsInGlobList() seem like good candidates for some unit tests. The "look" obvious, but a few boundary condition tests would be useful I think (filenames with spaces, backslashes, special characters, and relative & absolute paths are also an interesting thought here)." There aren't that many boundary conditions on a list with regexps, if you assume the regexp match function itself works. However, the tests is useful in documenting expectations. Fixes: #901 Suggested-by: James O.D. Hunt Signed-off-by: Christophe de Dinechin --- .../virtcontainers/pkg/oci/utils_test.go | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/runtime/virtcontainers/pkg/oci/utils_test.go b/src/runtime/virtcontainers/pkg/oci/utils_test.go index 6190203649..9e1a4e5b3c 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils_test.go +++ b/src/runtime/virtcontainers/pkg/oci/utils_test.go @@ -895,6 +895,34 @@ func TestAddRuntimeAnnotations(t *testing.T) { assert.Equal(config.NetworkConfig.InterworkingModel, vc.NetXConnectMacVtapModel) } +func TestRegexpContains(t *testing.T) { + assert := assert.New(t) + + type testData struct { + regexps []string + toMatch string + expected bool + } + + data := []testData{ + {[]string{}, "", false}, + {[]string{}, "nonempty", false}, + {[]string{"simple"}, "simple", true}, + {[]string{"simple"}, "some_simple_text", true}, + {[]string{"simple"}, "simp", false}, + {[]string{"one", "two"}, "one", true}, + {[]string{"one", "two"}, "two", true}, + {[]string{"o*"}, "oooo", true}, + {[]string{"o*"}, "oooa", true}, + {[]string{"^o*$"}, "oooa", false}, + } + + for _, d := range data { + matched := regexpContains(d.regexps, d.toMatch) + assert.Equal(d.expected, matched, "%+v", d) + } +} + func TestIsCRIOContainerManager(t *testing.T) { assert := assert.New(t) From b2b3bc7ad86ba4cf2b51f53454cba7905589079f Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 11 Sep 2020 12:59:53 +0200 Subject: [PATCH 22/24] annotations: Add unit test for checkPathIsInGlobs There are a few interesting corner cases to consider for this function. Fixes: #901 Suggested-by: James O.D. Hunt Signed-off-by: Christophe de Dinechin --- .../virtcontainers/pkg/oci/utils_test.go | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/runtime/virtcontainers/pkg/oci/utils_test.go b/src/runtime/virtcontainers/pkg/oci/utils_test.go index 9e1a4e5b3c..53f9e02e6d 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils_test.go +++ b/src/runtime/virtcontainers/pkg/oci/utils_test.go @@ -923,6 +923,37 @@ func TestRegexpContains(t *testing.T) { } } +func TestCheckPathIsInGlobs(t *testing.T) { + assert := assert.New(t) + + type testData struct { + globs []string + toMatch string + expected bool + } + + data := []testData{ + {[]string{}, "", false}, + {[]string{}, "nonempty", false}, + {[]string{"simple"}, "simple", false}, + {[]string{"simple"}, "some_simple_text", false}, + {[]string{"/bin/ls"}, "/bin/ls", true}, + {[]string{"/bin/ls", "/bin/false"}, "/bin/ls", true}, + {[]string{"/bin/ls", "/bin/false"}, "/bin/false", true}, + {[]string{"/bin/ls", "/bin/false"}, "/bin/bar", false}, + {[]string{"/bin/*ls*"}, "/bin/ls", true}, + {[]string{"/bin/*ls*"}, "/bin/false", true}, + {[]string{"bin/ls"}, "/bin/ls", false}, + {[]string{"./bin/ls"}, "/bin/ls", false}, + {[]string{"*/bin/ls"}, "/bin/ls", false}, + } + + for _, d := range data { + matched := checkPathIsInGlobs(d.globs, d.toMatch) + assert.Equal(d.expected, matched, "%+v", d) + } +} + func TestIsCRIOContainerManager(t *testing.T) { assert := assert.New(t) From 398d79184cc568a7be974b7c97f0c65ab36ac797 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 11 Sep 2020 12:52:10 +0200 Subject: [PATCH 23/24] annotations: Split addHypervisorOverrides to reduce complexity Warning from gocyclo during make check: virtcontainers/pkg/oci/utils.go:404:1: cyclomatic complexity 37 of func `addHypervisorConfigOverrides` is high (> 30) (gocyclo) func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, runtime RuntimeConfig) error { ^ Fixes: #901 Signed-off-by: Christophe de Dinechin --- src/runtime/virtcontainers/pkg/oci/utils.go | 67 ++++++++++++--------- 1 file changed, 37 insertions(+), 30 deletions(-) diff --git a/src/runtime/virtcontainers/pkg/oci/utils.go b/src/runtime/virtcontainers/pkg/oci/utils.go index 6e3e272176..755e3c6b45 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils.go +++ b/src/runtime/virtcontainers/pkg/oci/utils.go @@ -416,36 +416,8 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, return err } - if value, ok := ocispec.Annotations[vcAnnotations.HypervisorPath]; ok { - if !checkPathIsInGlobs(runtime.HypervisorConfig.HypervisorPathList, value) { - return fmt.Errorf("hypervisor %v required from annotation is not valid", value) - } - config.HypervisorConfig.HypervisorPath = value - } - - if value, ok := ocispec.Annotations[vcAnnotations.JailerPath]; ok { - if !checkPathIsInGlobs(runtime.HypervisorConfig.JailerPathList, value) { - return fmt.Errorf("jailer %v required from annotation is not valid", value) - } - config.HypervisorConfig.JailerPath = value - } - - if value, ok := ocispec.Annotations[vcAnnotations.CtlPath]; ok { - if !checkPathIsInGlobs(runtime.HypervisorConfig.HypervisorCtlPathList, value) { - return fmt.Errorf("hypervisor control %v required from annotation is not valid", value) - } - config.HypervisorConfig.HypervisorCtlPath = value - } - - if value, ok := ocispec.Annotations[vcAnnotations.KernelParams]; ok { - if value != "" { - params := vc.DeserializeParams(strings.Fields(value)) - for _, param := range params { - if err := config.HypervisorConfig.AddKernelParam(param); err != nil { - return fmt.Errorf("Error adding kernel parameters in annotation kernel_params : %v", err) - } - } - } + if err := addHypervisorPathOverrides(ocispec, config, runtime); err != nil { + return err } if value, ok := ocispec.Annotations[vcAnnotations.MachineType]; ok { @@ -522,6 +494,41 @@ func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, return nil } +func addHypervisorPathOverrides(ocispec specs.Spec, config *vc.SandboxConfig, runtime RuntimeConfig) error { + if value, ok := ocispec.Annotations[vcAnnotations.HypervisorPath]; ok { + if !checkPathIsInGlobs(runtime.HypervisorConfig.HypervisorPathList, value) { + return fmt.Errorf("hypervisor %v required from annotation is not valid", value) + } + config.HypervisorConfig.HypervisorPath = value + } + + if value, ok := ocispec.Annotations[vcAnnotations.JailerPath]; ok { + if !checkPathIsInGlobs(runtime.HypervisorConfig.JailerPathList, value) { + return fmt.Errorf("jailer %v required from annotation is not valid", value) + } + config.HypervisorConfig.JailerPath = value + } + + if value, ok := ocispec.Annotations[vcAnnotations.CtlPath]; ok { + if !checkPathIsInGlobs(runtime.HypervisorConfig.HypervisorCtlPathList, value) { + return fmt.Errorf("hypervisor control %v required from annotation is not valid", value) + } + config.HypervisorConfig.HypervisorCtlPath = value + } + + if value, ok := ocispec.Annotations[vcAnnotations.KernelParams]; ok { + if value != "" { + params := vc.DeserializeParams(strings.Fields(value)) + for _, param := range params { + if err := config.HypervisorConfig.AddKernelParam(param); err != nil { + return fmt.Errorf("Error adding kernel parameters in annotation kernel_params : %v", err) + } + } + } + } + return nil +} + func addHypervisorMemoryOverrides(ocispec specs.Spec, sbConfig *vc.SandboxConfig, runtime RuntimeConfig) error { if value, ok := ocispec.Annotations[vcAnnotations.DefaultMemory]; ok { memorySz, err := strconv.ParseUint(value, 10, 32) From c5771be2de82f06b6ff4f4df8b98c77860ddc900 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 11 Sep 2020 14:13:45 +0200 Subject: [PATCH 24/24] annotations: Correct unit tests to validate new protections Add the verification of some basic protections, namely that: - EnableAnnotations is honored - Dangerous paths cannot be modified if no match - Errors are returned when expected Fixes: #901 Signed-off-by: Christophe de Dinechin --- .../virtcontainers/pkg/oci/utils_test.go | 83 ++++++++++++++++++- 1 file changed, 80 insertions(+), 3 deletions(-) diff --git a/src/runtime/virtcontainers/pkg/oci/utils_test.go b/src/runtime/virtcontainers/pkg/oci/utils_test.go index 53f9e02e6d..a813bfae52 100644 --- a/src/runtime/virtcontainers/pkg/oci/utils_test.go +++ b/src/runtime/virtcontainers/pkg/oci/utils_test.go @@ -681,7 +681,20 @@ func TestAddAssetAnnotations(t *testing.T) { Console: consolePath, } - addAnnotations(ocispec, &config, runtimeConfig) + // Try annotations without enabling them first + err := addAnnotations(ocispec, &config, runtimeConfig) + assert.Error(err) + assert.Exactly(map[string]string{}, config.Annotations) + + // Check if annotation not enabled correctly + runtimeConfig.HypervisorConfig.EnableAnnotations = []string{"nonexistent"} + err = addAnnotations(ocispec, &config, runtimeConfig) + assert.Error(err) + + // Check that it works if all annotation are enabled + runtimeConfig.HypervisorConfig.EnableAnnotations = []string{".*"} + err = addAnnotations(ocispec, &config, runtimeConfig) + assert.NoError(err) assert.Exactly(expectedAnnotations, config.Annotations) } @@ -771,6 +784,9 @@ func TestAddHypervisorAnnotations(t *testing.T) { HypervisorType: vc.QemuHypervisor, Console: consolePath, } + runtimeConfig.HypervisorConfig.EnableAnnotations = []string{".*"} + runtimeConfig.HypervisorConfig.FileBackedMemRootList = []string{"/dev/shm*"} + runtimeConfig.HypervisorConfig.VirtioFSDaemonList = []string{"/bin/*ls*"} ocispec.Annotations[vcAnnotations.KernelParams] = "vsyscall=emulate iommu=on" addHypervisorConfigOverrides(ocispec, &config, runtimeConfig) @@ -794,7 +810,7 @@ func TestAddHypervisorAnnotations(t *testing.T) { ocispec.Annotations[vcAnnotations.BlockDeviceCacheDirect] = "true" ocispec.Annotations[vcAnnotations.BlockDeviceCacheNoflush] = "true" ocispec.Annotations[vcAnnotations.SharedFS] = "virtio-fs" - ocispec.Annotations[vcAnnotations.VirtioFSDaemon] = "/home/virtiofsd" + ocispec.Annotations[vcAnnotations.VirtioFSDaemon] = "/bin/false" ocispec.Annotations[vcAnnotations.VirtioFSCache] = "/home/cache" ocispec.Annotations[vcAnnotations.Msize9p] = "512" ocispec.Annotations[vcAnnotations.MachineType] = "q35" @@ -831,7 +847,7 @@ func TestAddHypervisorAnnotations(t *testing.T) { assert.Equal(config.HypervisorConfig.BlockDeviceCacheDirect, true) assert.Equal(config.HypervisorConfig.BlockDeviceCacheNoflush, true) assert.Equal(config.HypervisorConfig.SharedFS, "virtio-fs") - assert.Equal(config.HypervisorConfig.VirtioFSDaemon, "/home/virtiofsd") + assert.Equal(config.HypervisorConfig.VirtioFSDaemon, "/bin/false") assert.Equal(config.HypervisorConfig.VirtioFSCache, "/home/cache") assert.Equal(config.HypervisorConfig.Msize9p, uint32(512)) assert.Equal(config.HypervisorConfig.HypervisorMachineType, "q35") @@ -867,6 +883,67 @@ func TestAddHypervisorAnnotations(t *testing.T) { assert.Error(err) } +func TestAddProtectedHypervisorAnnotations(t *testing.T) { + assert := assert.New(t) + + config := vc.SandboxConfig{ + Annotations: make(map[string]string), + } + + ocispec := specs.Spec{ + Annotations: make(map[string]string), + } + + runtimeConfig := RuntimeConfig{ + HypervisorType: vc.QemuHypervisor, + Console: consolePath, + } + ocispec.Annotations[vcAnnotations.KernelParams] = "vsyscall=emulate iommu=on" + err := addAnnotations(ocispec, &config, runtimeConfig) + assert.Error(err) + assert.Exactly(vc.HypervisorConfig{}, config.HypervisorConfig) + + // Enable annotations + runtimeConfig.HypervisorConfig.EnableAnnotations = []string{".*"} + + ocispec.Annotations[vcAnnotations.FileBackedMemRootDir] = "/dev/shm" + ocispec.Annotations[vcAnnotations.VirtioFSDaemon] = "/bin/false" + + config.HypervisorConfig.FileBackedMemRootDir = "do-not-touch" + config.HypervisorConfig.VirtioFSDaemon = "dangerous-daemon" + + err = addAnnotations(ocispec, &config, runtimeConfig) + assert.Error(err) + assert.Equal(config.HypervisorConfig.FileBackedMemRootDir, "do-not-touch") + assert.Equal(config.HypervisorConfig.VirtioFSDaemon, "dangerous-daemon") + + // Now enable them and check again + runtimeConfig.HypervisorConfig.FileBackedMemRootList = []string{"/dev/*m"} + runtimeConfig.HypervisorConfig.VirtioFSDaemonList = []string{"/bin/*ls*"} + err = addAnnotations(ocispec, &config, runtimeConfig) + assert.NoError(err) + assert.Equal(config.HypervisorConfig.FileBackedMemRootDir, "/dev/shm") + assert.Equal(config.HypervisorConfig.VirtioFSDaemon, "/bin/false") + + // In case an absurd large value is provided, the config value if not over-ridden + ocispec.Annotations[vcAnnotations.DefaultVCPUs] = "655536" + err = addAnnotations(ocispec, &config, runtimeConfig) + assert.Error(err) + + ocispec.Annotations[vcAnnotations.DefaultVCPUs] = "-1" + err = addAnnotations(ocispec, &config, runtimeConfig) + assert.Error(err) + + ocispec.Annotations[vcAnnotations.DefaultVCPUs] = "1" + ocispec.Annotations[vcAnnotations.DefaultMaxVCPUs] = "-1" + err = addAnnotations(ocispec, &config, runtimeConfig) + assert.Error(err) + + ocispec.Annotations[vcAnnotations.DefaultMaxVCPUs] = "1" + ocispec.Annotations[vcAnnotations.DefaultMemory] = fmt.Sprintf("%d", vc.MinHypervisorMemory+1) + assert.Error(err) +} + func TestAddRuntimeAnnotations(t *testing.T) { assert := assert.New(t)