Re: [libvirt] [PATCH v2 1/5] security, apparmor: add (Set|Restore)MemoryLabel

On Wed, 2018-03-21 at 13:10 +0100, Christian Ehrhardt wrote: > Recent changes have made implementing this mandatory to hot add any > memory. > Implementing this in apparmor fixes this as well as allows hot-add of > nvdimm > tpye memory with an nvdimmPath set generating a AppArmor rule for > that > path. > > Example hot adding: > <memory model='nvdimm'> > <source> > <path>/tmp/nvdimm-test</path> > </source> > <target> > <size unit='KiB'>524288</size> > <node>0</node> > </target> > </memory> > Creates now: > "/tmp/nvdimm-test" rwk, > > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1755153 > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com&gt; > --- > src/security/security_apparmor.c | 43 > ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/src/security/security_apparmor.c > b/src/security/security_apparmor.c > index a989992..7509552 100644 > --- a/src/security/security_apparmor.c > +++ b/src/security/security_apparmor.c > @@ -718,6 +718,46 @@ > AppArmorRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, > > /* Called when hotplugging */ > static int > +AppArmorSetMemoryLabel(virSecurityManagerPtr mgr, > + virDomainDefPtr def, > + virDomainMemoryDefPtr mem) > +{ > + switch ((virDomainMemoryModel) mem->model) { Perhaps check if mem->model is not NULL? Sorry for not noticing this before... yeah could be a good cautious thing to do as well.

> + case VIR_DOMAIN_MEMORY_MODEL_NVDIMM: > + if (!virFileExists(mem->nvdimmPath)) { and the same for mem->nvdimmPath?

> + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("%s: \'%s\' does not exist"), > + __func__, mem->nvdimmPath); > + return -1; > + } > + return reload_profile(mgr, def, mem->nvdimmPath, true); > + break; > + case VIR_DOMAIN_MEMORY_MODEL_NONE: > + case VIR_DOMAIN_MEMORY_MODEL_DIMM: > + case VIR_DOMAIN_MEMORY_MODEL_LAST: > + break; > + } > + > + return 0; > +} > + > + > +static int > +AppArmorRestoreMemoryLabel(virSecurityManagerPtr mgr, > + virDomainDefPtr def, > + virDomainMemoryDefPtr mem > ATTRIBUTE_UNUSED) > +{ > + virSecurityLabelDefPtr secdef = > + virDomainDefGetSecurityLabelDef(def, > SECURITY_APPARMOR_NAME); > + > + if (!secdef || !secdef->relabel) > + return 0; > + You forgot to remove the secdef tests here too (they are already in reload_profile).

> + return reload_profile(mgr, def, NULL, false); > +} > + > +/* Called when hotplugging */ > +static int > AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, > virDomainDefPtr def, > virStorageSourcePtr src) > @@ -1115,6 +1155,9 @@ virSecurityDriver virAppArmorSecurityDriver = { > .domainSetSecurityImageLabel = > AppArmorSetSecurityImageLabel, > .domainRestoreSecurityImageLabel = > AppArmorRestoreSecurityImageLabel, > > + .domainSetSecurityMemoryLabel = AppArmorSetMemoryLabel, > + .domainRestoreSecurityMemoryLabel = > AppArmorRestoreMemoryLabel, > + > .domainSetSecurityDaemonSocketLabel = > AppArmorSetSecurityDaemonSocketLabel, > .domainSetSecuritySocketLabel = > AppArmorSetSecuritySocketLabel, > .domainClearSecuritySocketLabel = > AppArmorClearSecuritySocketLabel, -- Jamie Strandboge | http://www.canonical.com