[apparmor] [PATCH 6/6] libapparmor: Allow creating a kernel_interface with a NULL kernel_features

[Tyler Hicks]

On 2015-03-26 00:32:26, Steve Beattie wrote:
> On Wed, Mar 25, 2015 at 05:37:21PM -0500, Tyler Hicks wrote:
> > The most common case when creating an aa_kernel_interface object will be
> > to do so while using the current kernel's feature set for the
> > kernel_features parameter. Rather than have callers instantiate their
> > own aa_features object in this situation, aa_kernel_interface_new()
> > should do it for them if they specify NULL for the kernel_features
> > parameter.
> > 
> > Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> 
> And similarly for this one, Acked-by: Steve Beattie <steve at nxnw.org>.
> 
> With this and the previous change, all the calls to
> aa_features_new_from_kernel() in the test source aa_policy_cache.c
> are gone. Do you envision a situation where a user of the API would
> still want to get the features structure from the running kernel
> via aa_features_new_from_kernel()?  Or can we make it an internal
> function to the library, simplifying the interface?

This is a good question. It feels like something we might need in the
future when we're wanting to check if the features of the kernel that
was just installed matches the features of the kernel that is running.
However, I'm not sure that we would need to make that check. Instead,
we'd probably be checking if the kernel that was just installed has a
valid policy cache directory that already exists, regardless of what
kernel is currently running.

The parser does need to be able to call aa_features_new_from_kernel()
because it needs to do support checks on various things such as if the
current kernel supports ptrace mediation. That wouldn't keep us from
making it an exported-by-private function but I'm not sure that buys us
anything because we will have an external-to-libapparmor user of the
function even if it is our own parser...

Tyler

> 
> > ---
> >  libraries/libapparmor/src/kernel_interface.c | 18 ++++++++++++++----
> >  tests/regression/apparmor/aa_policy_cache.c  |  9 +--------
> >  2 files changed, 15 insertions(+), 12 deletions(-)
> > 
> > diff --git a/libraries/libapparmor/src/kernel_interface.c b/libraries/libapparmor/src/kernel_interface.c
> > index 6ab20ea..15b171f 100644
> > --- a/libraries/libapparmor/src/kernel_interface.c
> > +++ b/libraries/libapparmor/src/kernel_interface.c
> > @@ -198,7 +198,9 @@ static int write_policy_file_to_iface(aa_kernel_interface *kernel_interface,
> >   * aa_kernel_interface_new - create a new kernel_interface from an optional path
> >   * @kernel_interface: will point to the address of an allocated and initialized
> >   *                    aa_kernel_interface object upon success
> > - * @kernel_features: features representing the currently running kernel
> > + * @kernel_features: features representing the currently running kernel (can be
> > + *                   NULL and the features of the currently running kernel will
> > + *                   be used)
> >   * @apparmorfs: path to the apparmor directory of the mounted securityfs (can
> >   *              be NULL and the path will be auto discovered)
> >   *
> > @@ -223,9 +225,17 @@ int aa_kernel_interface_new(aa_kernel_interface **kernel_interface,
> >  	aa_kernel_interface_ref(ki);
> >  	ki->dirfd = -1;
> >  
> > -	ki->supports_setload = kernel_features ?
> > -			       aa_features_supports(kernel_features, set_load) :
> > -			       false;
> > +	if (kernel_features) {
> > +		aa_features_ref(kernel_features);
> > +	} else if (aa_features_new_from_kernel(&kernel_features) == -1) {
> > +		int save = errno;
> > +
> > +		aa_kernel_interface_unref(ki);
> > +		errno = save;
> > +		return -1;
> > +	}
> > +	ki->supports_setload = aa_features_supports(kernel_features, set_load);
> > +	aa_features_unref(kernel_features);
> >  
> >  	if (!apparmorfs) {
> >  		if (find_iface_dir(&alloced_apparmorfs) == -1) {
> > diff --git a/tests/regression/apparmor/aa_policy_cache.c b/tests/regression/apparmor/aa_policy_cache.c
> > index 03f7b5b..b221c98 100644
> > --- a/tests/regression/apparmor/aa_policy_cache.c
> > +++ b/tests/regression/apparmor/aa_policy_cache.c
> > @@ -120,16 +120,10 @@ out:
> >  
> >  static int test_remove_policy(const char *name)
> >  {
> > -	aa_features *features = NULL;
> >  	aa_kernel_interface *kernel_interface = NULL;
> >  	int rc = 1;
> >  
> > -	if (aa_features_new_from_kernel(&features)) {
> > -		perror("FAIL - aa_features_new_from_kernel");
> > -		goto out;
> > -	}
> > -
> > -	if (aa_kernel_interface_new(&kernel_interface, features, NULL)) {
> > +	if (aa_kernel_interface_new(&kernel_interface, NULL, NULL)) {
> >  		perror("FAIL - aa_kernel_interface_new");
> >  		goto out;
> >  	}
> > @@ -142,7 +136,6 @@ static int test_remove_policy(const char *name)
> >  	rc = 0;
> >  out:
> >  	aa_kernel_interface_unref(kernel_interface);
> > -	aa_features_unref(features);
> >  	return rc;
> >  }
> 
> -- 
> Steve Beattie
> <sbeattie at ubuntu.com>
> http://NxNW.org/~steve/



> -- 
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150326/73011617/attachment.pgp>