linux-kernel - Re: [PATCH 3/3] p9auth: add p9auth driver

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:	Tue, 20 Apr 2010 20:04:06 -0700
From:	Greg KH <greg@...ah.com>
To:	"Serge E. Hallyn" <serue@...ibm.com>
Cc:	lkml <linux-kernel@...r.kernel.org>,
	David Howells <dhowells@...hat.com>,
	Ashwin Ganti <ashwin.ganti@...il.com>, rsc@...ch.com,
	ericvh@...il.com, linux-security-module@...r.kernel.org,
	Ron Minnich <rminnich@...il.com>, jt.beard@...il.com,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andrew Morgan <morgan@...nel.org>, oleg@...ibm.com,
	Eric Paris <eparis@...hat.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	linux-api@...r.kernel.org, Randy Dunlap <rdunlap@...otime.net>
Subject: Re: [PATCH 3/3] p9auth: add p9auth driver

On Tue, Apr 20, 2010 at 08:29:08PM -0500, Serge E. Hallyn wrote:
> This is a driver that adds Plan 9 style capability device
> implementation.  See Documentation/p9auth.txt for a description
> of how to use this.

Hm, you didn't originally write this driver, so it would be good to get
some original authorship information in here to keep everything correct,
right?

>  Documentation/p9auth.txt     |   47 ++++
>  drivers/char/Kconfig         |    2 +
>  drivers/char/Makefile        |    2 +
>  drivers/char/p9auth/Kconfig  |    9 +
>  drivers/char/p9auth/Makefile |    1 +
>  drivers/char/p9auth/p9auth.c |  517 ++++++++++++++++++++++++++++++++++++++++++

Is this code really ready for drivers/char/?  What has changed in it
that makes it ok to move out of the staging tree?

And who is going to maintain it?  You?  Or someone else?

>  6 files changed, 578 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/p9auth.txt
>  create mode 100644 drivers/char/p9auth/Kconfig
>  create mode 100644 drivers/char/p9auth/Makefile
>  create mode 100644 drivers/char/p9auth/p9auth.c
> 
> diff --git a/Documentation/p9auth.txt b/Documentation/p9auth.txt
> new file mode 100644
> index 0000000..14a69d8
> --- /dev/null
> +++ b/Documentation/p9auth.txt
> @@ -0,0 +1,47 @@
> +The p9auth device driver implements a plan-9 factotum-like
> +capability API.  Tasks which are privileged (authorized by
> +possession of the CAP_GRANT_ID privilege (POSIX capability))
> +can write new capabilities to /dev/caphash.  The kernel then
> +stores these until a task uses them by writing to the
> +/dev/capuse device.  Each capability represents the ability
> +for a task running as userid X to switch to userid Y and
> +some set of groups.  Each capability may be used only once,
> +and unused capabilities are cleared after two minutes.
> +
> +The following examples shows how to use the API.  Shell 1
> +contains a privileged root shell.  Shell 2 contains an
> +unprivileged shell as user 501 in the same user namespace.  If
> +not already done, the privileged shell should create the p9auth
> +devices:
> +
> +	majfile=/sys/module/p9auth/parameters/cap_major
> +	minfile=/sys/module/p9auth/parameters/cap_minor
> +	maj=`cat $majfile`
> +	mknod /dev/caphash c $maj 0
> +	min=`cat $minfile`
> +	mknod /dev/capuse c $maj 1
> +	chmod ugo+w /dev/capuse

That is incorrect, you don't need the cap_major/minor files at all, the
device node should be automatically created for you, right?

And do you really want to do all of this control through a device node?
Why?

> +DEFINE_MUTEX(cap_mutex); /* TODO fix up the locking one day */

One might hope that today would be that day...

Also, please run this through sparse.  This is a variable that doesn't
need to be global.

> +struct cap_dev {
> +	struct cdev cdev;
> +};

Do you really need to do it this way?  A cdev for a single device node?
That's major overkill.

> +static int cap_major = CAP_MAJOR;
> +static int cap_minor;

Just always use a dynamic misc device, you never need a whole major for
this.

> +module_param(cap_major, int, S_IRUGO);
> +module_param(cap_minor, int, S_IRUGO);

Can be removed.

> +MODULE_AUTHOR("Ashwin Ganti");

Hm, who is going to maintain this, you, or Ashwin?

> +static void hexdump(unsigned char *buf, unsigned int len)
> +{
> +	while (len--)
> +		printk(KERN_DEBUG "%02x", *buf++);
> +	printk(KERN_DEBUG "\n");
> +}

We have a built-in function for this already.

Oh, this function is also incorrect, which is a good reason to use the
built-in ones.

I'm going to stop now, this doesn't belong in drivers/char/ yet, it
needs work...

thanks,

greg k-h

> +/*
> + * read an entry.  For now it is
> + * source_user@...get_user@...d
> + * Next it will become
> + * source_user@...get_user@...get_group@...groups@...1..@...n@...d
> + */

Hm, wait, one more....  The kernel/user api is going to change some time
in the future?  Please fix this now before it gets merged.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/