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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 1 Feb 2023 11:46:47 -0800
From:   Fan Wu <wufan@...ux.microsoft.com>
To:     Roberto Sassu <roberto.sassu@...weicloud.com>
Cc:     corbet@....net, zohar@...ux.ibm.com, jmorris@...ei.org,
        serge@...lyn.com, tytso@....edu, ebiggers@...nel.org,
        axboe@...nel.dk, agk@...hat.com, snitzer@...nel.org,
        eparis@...hat.com, paul@...l-moore.com, linux-doc@...r.kernel.org,
        linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-fscrypt@...r.kernel.org, linux-block@...r.kernel.org,
        dm-devel@...hat.com, linux-audit@...hat.com,
        roberto.sassu@...wei.com, linux-kernel@...r.kernel.org,
        Deven Bowers <deven.desai@...ux.microsoft.com>
Subject: Re: [RFC PATCH v9 05/16] ipe: add userspace interface

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@...ux.microsoft.com>
> > +
> > +/**
> > + * 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