Re: [RFC PATCH v9 05/16] ipe: add userspace interface

[Fan Wu]

On Tue, Jan 31, 2023 at 11:49:44AM +0100, Roberto Sassu wrote:
> On Mon, 2023-01-30 at 14:57 -0800, Fan Wu wrote:
> > From: Deven Bowers <deven.desai@xxxxxxxxxxxxxxxxxxx>
> > +
> > +/**
> > + * new_policy - Write handler for the securityfs node, "ipe/new_policy".
> > + * @f: Supplies a file structure representing the securityfs node.
> > + * @data: Suppleis a buffer passed to the write syscall.
> 
> Typo: Suppleis.
> 
Thanks for spotting the typos!

> > + * @len: Supplies the length of @data.
> > + * @offset: unused.
> > + *
> > + * Return:
> > + * * >0	- Success, Length of buffer written
> > + * * <0	- Error
> > + */
> > +static ssize_t new_policy(struct file *f, const char __user *data,
> > +			  size_t len, loff_t *offset)
> > +{
> > +	int rc = 0;
> > +	char *copy = NULL;
> > +	struct ipe_policy *p = NULL;
> > +
> > +	if (!file_ns_capable(f, &init_user_ns, CAP_MAC_ADMIN))
> > +		return -EPERM;
> > +
> > +	copy = memdup_user_nul(data, len);
> > +	if (IS_ERR(copy)) {
> > +		rc = PTR_ERR(copy);
> > +		goto err;
> > +	}
> > +
> > +	p = ipe_new_policy(NULL, 0, copy, len);
> > +	if (IS_ERR(p)) {
> > +		rc = PTR_ERR(p);
> > +		goto err;
> > +	}
> > +
> > +	rc = ipe_new_policyfs_node(p);
> > +	if (rc)
> > +		goto err;
> 
> Uhm, don't you need to do cleanup of allocated memory or revert the
> actions of ipe_new_policy()?
> 
Yes that should be cleaned up but should be done in ipe_new_policy instead,
will add a ipe_free_policy call at the end. Thanks for pointing that out.

> 
> I would like more to see all the functions managing the policy
> together. If the patch is too long, you could further split by adding
> the helpers (that don't directly deal with the policy) in a separate
> patch.
> 
> Here you would simply instantiate dirs/files in securityfs and call the
> existing functions previously introduced.
> 
> Roberto
> 

I will try to split them in the next version. Thanks for the suggestion.
-Fan