[PATCH] devpts: allow mounting with uid/gid of uint32_t

[PATCH] devpts: allow mounting with uid/gid of uint32_t

[Dongsu Park]

To allow devpts to be mounted with options of uid/gid of uint32_t,
use kstrtouint() instead of match_int(). Doing that, mounting devpts
with uid or gid > (2^31 - 1) will work as expected, e.g.:

 # mount -t devpts devpts /tmp/devptsdir -o \
   newinstance,ptmxmode=0666,mode=620,uid=3598450688,gid=3598450693

It was originally by reported on systemd github issues:
https://github.com/systemd/systemd/issues/956

Reported-by: Alban Crequy <alban@endocode.com>
Signed-off-by: Dongsu Park <dpark@posteo.net>
---
 fs/devpts/inode.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index c35ffdc12bba..83c3e7368f38 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -188,23 +188,33 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 		token = match_token(p, tokens, args);
 		switch (token) {
 		case Opt_uid:
-			if (match_int(&args[0], &option))
+		{
+			char *uidstr = args[0].from;
+			uid_t uidval;
+			int rc = kstrtouint(uidstr, 0, &uidval);
+			if (rc)
 				return -EINVAL;
-			uid = make_kuid(current_user_ns(), option);
+			uid = make_kuid(current_user_ns(), uidval);
 			if (!uid_valid(uid))
 				return -EINVAL;
 			opts->uid = uid;
 			opts->setuid = 1;
 			break;
+                }
 		case Opt_gid:
-			if (match_int(&args[0], &option))
+                {
+			char *gidstr = args[0].from;
+			gid_t gidval;
+			int rc = kstrtouint(gidstr, 0, &gidval);
+			if (rc)
 				return -EINVAL;
-			gid = make_kgid(current_user_ns(), option);
+			gid = make_kgid(current_user_ns(), gidval);
 			if (!gid_valid(gid))
 				return -EINVAL;
 			opts->gid = gid;
 			opts->setgid = 1;
 			break;
+                }
 		case Opt_mode:
 			if (match_octal(&args[0], &option))
 				return -EINVAL;
-- 
2.1.0

[PATCH v2] devpts: allow mounting with uid/gid of uint32_t

[Dongsu Park]

To allow devpts to be mounted with options of uid/gid of uint32_t,
use kstrtouint() instead of match_int(). Doing that, mounting devpts
with uid or gid > (2^31 - 1) will work as expected, e.g.:

 # mount -t devpts devpts /tmp/devptsdir -o \
   newinstance,ptmxmode=0666,mode=620,uid=3598450688,gid=3598450693

It was originally by reported on systemd github issues:
https://github.com/systemd/systemd/issues/956

from v1: fix patch format correctly

Reported-by: Alban Crequy <alban@endocode.com>
Signed-off-by: Dongsu Park <dpark@posteo.net>
---
 fs/devpts/inode.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index c35ffdc12bba..49272fae40a7 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -188,23 +188,35 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 		token = match_token(p, tokens, args);
 		switch (token) {
 		case Opt_uid:
-			if (match_int(&args[0], &option))
+		{
+			char *uidstr = args[0].from;
+			uid_t uidval;
+			int rc = kstrtouint(uidstr, 0, &uidval);
+
+			if (rc)
 				return -EINVAL;
-			uid = make_kuid(current_user_ns(), option);
+			uid = make_kuid(current_user_ns(), uidval);
 			if (!uid_valid(uid))
 				return -EINVAL;
 			opts->uid = uid;
 			opts->setuid = 1;
 			break;
+		}
 		case Opt_gid:
-			if (match_int(&args[0], &option))
+		{
+			char *gidstr = args[0].from;
+			gid_t gidval;
+			int rc = kstrtouint(gidstr, 0, &gidval);
+
+			if (rc)
 				return -EINVAL;
-			gid = make_kgid(current_user_ns(), option);
+			gid = make_kgid(current_user_ns(), gidval);
 			if (!gid_valid(gid))
 				return -EINVAL;
 			opts->gid = gid;
 			opts->setgid = 1;
 			break;
+		}
 		case Opt_mode:
 			if (match_octal(&args[0], &option))
 				return -EINVAL;
-- 
2.1.0

Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t

[Andrew Morton]

On Tue, 18 Aug 2015 17:18:19 +0200 Dongsu Park <dpark@posteo.net> wrote:

> To allow devpts to be mounted with options of uid/gid of uint32_t,
> use kstrtouint() instead of match_int(). Doing that, mounting devpts
> with uid or gid > (2^31 - 1) will work as expected, e.g.:
> 
>  # mount -t devpts devpts /tmp/devptsdir -o \
>    newinstance,ptmxmode=0666,mode=620,uid=3598450688,gid=3598450693
> 
> It was originally by reported on systemd github issues:
> https://github.com/systemd/systemd/issues/956
> 
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -188,23 +188,35 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
>  		token = match_token(p, tokens, args);
>  		switch (token) {
>  		case Opt_uid:
> -			if (match_int(&args[0], &option))
> +		{

It might be neater to lay this out as

  		case Opt_uid: {

> +			char *uidstr = args[0].from;
> +			uid_t uidval;
> +			int rc = kstrtouint(uidstr, 0, &uidval);

This assumes that the architecture/config uses a uint for uid_t.  We
have no business assuming this - it's an opaque type for a reason.  It
would be safer to do

	unsigned long uidl;

	rc = kstrtoul(uidstr, 0, &uidl);
	uidval = uidl;

> +			if (rc)
>  				return -EINVAL;

I don't get it.  From my reading, kstrtouint->parse_integer() returns
"number of characters parsed or -E".  So this code won't work.  But
presumably it *does* work, so why?


Also, we should probably return `rc' here if it's negative, to
propagate the error which kstrtouint() detected.  That's a minor
non-back-compatible change but it shouldn't matter.

otoh, kstrtouint() likes to return -ERANGE when things go wrong. 
ERANGE means "Math result not representable", which is a nonsenscal
error code in this context.  Sigh, why do people keep doing this.


> -			uid = make_kuid(current_user_ns(), option);
> +			uid = make_kuid(current_user_ns(), uidval);
>  			if (!uid_valid(uid))
>  				return -EINVAL;
>  			opts->uid = uid;
>  			opts->setuid = 1;
>  			break;
>
> ...
>

Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t

[Dongsu Park]

Hi,

thanks for the review.

On 18.08.2015 16:44, Andrew Morton wrote:
> On Tue, 18 Aug 2015 17:18:19 +0200 Dongsu Park <dpark@posteo.net> wrote:
> 
> > To allow devpts to be mounted with options of uid/gid of uint32_t,
> > use kstrtouint() instead of match_int(). Doing that, mounting devpts
> > with uid or gid > (2^31 - 1) will work as expected, e.g.:
> > 
> >  # mount -t devpts devpts /tmp/devptsdir -o \
> >    newinstance,ptmxmode=0666,mode=620,uid=3598450688,gid=3598450693
> > 
> > It was originally by reported on systemd github issues:
> > https://github.com/systemd/systemd/issues/956
> > 
> > --- a/fs/devpts/inode.c
> > +++ b/fs/devpts/inode.c
> > @@ -188,23 +188,35 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
> >  		token = match_token(p, tokens, args);
> >  		switch (token) {
> >  		case Opt_uid:
> > -			if (match_int(&args[0], &option))
> > +		{
> 
> It might be neater to lay this out as
> 
>   		case Opt_uid: {

I'll do it.

> > +			char *uidstr = args[0].from;
> > +			uid_t uidval;
> > +			int rc = kstrtouint(uidstr, 0, &uidval);
> 
> This assumes that the architecture/config uses a uint for uid_t.  We
> have no business assuming this - it's an opaque type for a reason.  It
> would be safer to do
> 
> 	unsigned long uidl;
> 
> 	rc = kstrtoul(uidstr, 0, &uidl);
> 	uidval = uidl;

That's a good point. I'll do it.

> > +			if (rc)
> >  				return -EINVAL;
> 
> I don't get it.  From my reading, kstrtouint->parse_integer() returns
> "number of characters parsed or -E".  So this code won't work.  But
> presumably it *does* work, so why?

It's probably because kstrtouint() returns just 0 on success.
That's what functions in the call chain of kstrtouint() -> kstrtoull() ->
_kstrtoull() -> _parse_integer() are actually doing.
_parse_integer() actually returns rv, i.e. number of characters parsed.
But after that, if there's no error, _kstrtoull() simply returns 0.

> Also, we should probably return `rc' here if it's negative, to
> propagate the error which kstrtouint() detected.  That's a minor
> non-back-compatible change but it shouldn't matter.

Okay, I also think that we should return rc. I'll do it.

> otoh, kstrtouint() likes to return -ERANGE when things go wrong. 
> ERANGE means "Math result not representable", which is a nonsenscal
> error code in this context.  Sigh, why do people keep doing this.

Hmm, good to know.

Thanks,
Dongsu

> > -			uid = make_kuid(current_user_ns(), option);
> > +			uid = make_kuid(current_user_ns(), uidval);
> >  			if (!uid_valid(uid))
> >  				return -EINVAL;
> >  			opts->uid = uid;
> >  			opts->setuid = 1;
> >  			break;
> >
> > ...
> >

Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t

[Andrew Morton]

On Wed, 19 Aug 2015 09:24:31 +0200 Dongsu Park <dpark@posteo.net> wrote:

> > 	unsigned long uidl;
> > 
> > 	rc = kstrtoul(uidstr, 0, &uidl);
> > 	uidval = uidl;
> 
> That's a good point. I'll do it.
> 
> > > +			if (rc)
> > >  				return -EINVAL;
> > 
> > I don't get it.  From my reading, kstrtouint->parse_integer() returns
> > "number of characters parsed or -E".  So this code won't work.  But
> > presumably it *does* work, so why?
> 
> It's probably because kstrtouint() returns just 0 on success.
> That's what functions in the call chain of kstrtouint() -> kstrtoull() ->
> _kstrtoull() -> _parse_integer() are actually doing.
> _parse_integer() actually returns rv, i.e. number of characters parsed.
> But after that, if there's no error, _kstrtoull() simply returns 0.

whoa, wait, I was looking at the -mm tree which changes kstrtouint():

static inline int __must_check kstrtouint(const char *s, unsigned int base, unsigned int *res)
{
	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
}

and

 * Return number of characters parsed or -E.
 ...
 */
#define parse_integer(s, base, val)	\



Alexey, doesn't this mean that code which does

	if (kstrtouint(...))
		return -EFOO;

will break?  Is it intended that parse_integer-convert-*.patch will fix
every callsite in the kernel?  If so, how do we know there haven't been
concurrent additions in -next which need review/conversion?  Let alone
out-of-tree things...

Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t

[Rasmus Villemoes]

On Wed, Aug 19 2015, Andrew Morton <akpm@linux-foundation.org> wrote:

>
> whoa, wait, I was looking at the -mm tree which changes kstrtouint():
>
> static inline int __must_check kstrtouint(const char *s, unsigned int base, unsigned int *res)
> {
> 	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
> }
>
> and
>
>  * Return number of characters parsed or -E.
>  ...
>  */
> #define parse_integer(s, base, val)	\
>
>
> Alexey, doesn't this mean that code which does
>
> 	if (kstrtouint(...))
> 		return -EFOO;
>
> will break?

No, because PARSE_INTEGER_NEWLINE means more than just accepting a
trailing newline. It also requires the entire string to be consumed, and
changes the return semantics.

I suggested splitting those three things into separate flags and letting
PARSE_INTEGER_KSTRTOX be a shorthand for those.

<http://thread.gmane.org/gmane.linux.kernel/1949066/focus=1949239>

Rasmus

Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t

[Alexey Dobriyan]

On Wed, Aug 19, 2015 at 10:47 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 19 Aug 2015 09:24:31 +0200 Dongsu Park <dpark@posteo.net> wrote:
>
>> >     unsigned long uidl;
>> >
>> >     rc = kstrtoul(uidstr, 0, &uidl);
>> >     uidval = uidl;
>>
>> That's a good point. I'll do it.
>>
>> > > +                 if (rc)
>> > >                           return -EINVAL;
>> >
>> > I don't get it.  From my reading, kstrtouint->parse_integer() returns
>> > "number of characters parsed or -E".  So this code won't work.  But
>> > presumably it *does* work, so why?
>>
>> It's probably because kstrtouint() returns just 0 on success.
>> That's what functions in the call chain of kstrtouint() -> kstrtoull() ->
>> _kstrtoull() -> _parse_integer() are actually doing.
>> _parse_integer() actually returns rv, i.e. number of characters parsed.
>> But after that, if there's no error, _kstrtoull() simply returns 0.
>
> whoa, wait, I was looking at the -mm tree which changes kstrtouint():
>
> static inline int __must_check kstrtouint(const char *s, unsigned int base, unsigned int *res)
> {
>         return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
> }
>
> and
>
>  * Return number of characters parsed or -E.
>  ...
>  */
> #define parse_integer(s, base, val)     \
>
>
>
> Alexey, doesn't this mean that code which does
>
>         if (kstrtouint(...))
>                 return -EFOO;
>
> will break?

Nothing is supposed to break (even between patches in that series).
kstrto*() still returns 0 on success or -EINVAL/-ERANGE on error.

Commenting on other things in this thread:

> This assumes that the architecture/config
> uses a uint for uid_t. We have no business
> assuming this - it's an opaque type for a reason.
> It would be safer to do
>
> unsigned long uidl;
>
> rc = kstrtoul(uidstr, 0, &uidl);
> uidval = uidl;

This code is not safe at all because unsigned long is wider
than unsigned int. "4294967296" will be silently parsed as 0.
kstrtou32() should be used in this case. Yes, the names
do not match, but C types do. If uid_t as a type is changed,
compiler will notice immediately making kstrtou32() more safe.

> kstrtouint() likes to return -ERANGE when things go
> wrong. ERANGE means "Math result not representable",
> which is a nonsenscal error code in this context.

ERANGE is there to distinguish between "invalid" and
"valid but too big". Typical integer parsing code will accept
289367492873894273894729837428937489273
(a _very_ common bug).
C doesn't have bignums, so ERANGE exists.

And to teach people that -EINVAL is not the only error value,
so they won't hardcode it because in the future we might add
new error value because who knows why.

> PARSE_INTEGER_NEWLINE means more than
> just accepting a trailing newline.

Well, maybe it is misnamed, but it is internal implementation
detail. People aren't supposed to use it directly.
Name can be changed if it is so disturbing.

Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t

[Peter Hurley]

On 08/18/2015 11:18 AM, Dongsu Park wrote:
> To allow devpts to be mounted with options of uid/gid of uint32_t,
> use kstrtouint() instead of match_int(). Doing that, mounting devpts
> with uid or gid > (2^31 - 1) will work as expected, e.g.:
> 
>  # mount -t devpts devpts /tmp/devptsdir -o \
>    newinstance,ptmxmode=0666,mode=620,uid=3598450688,gid=3598450693
> 
> It was originally by reported on systemd github issues:
> https://github.com/systemd/systemd/issues/956
> 
> from v1: fix patch format correctly
> 
> Reported-by: Alban Crequy <alban@endocode.com>
> Signed-off-by: Dongsu Park <dpark@posteo.net>
> ---
>  fs/devpts/inode.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index c35ffdc12bba..49272fae40a7 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -188,23 +188,35 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
>  		token = match_token(p, tokens, args);
>  		switch (token) {
>  		case Opt_uid:
> -			if (match_int(&args[0], &option))

match_int() => make_kuid/kgid is a widespread pattern in filesystems
for handling uid/gid mount parameters.

How about adding a for-purpose string-to-uid/gid function, rather than
open-coding?

Regards,
Peter Hurley


> +		{
> +			char *uidstr = args[0].from;
> +			uid_t uidval;
> +			int rc = kstrtouint(uidstr, 0, &uidval);
> +
> +			if (rc)
>  				return -EINVAL;
> -			uid = make_kuid(current_user_ns(), option);
> +			uid = make_kuid(current_user_ns(), uidval);
>  			if (!uid_valid(uid))
>  				return -EINVAL;
>  			opts->uid = uid;
>  			opts->setuid = 1;
>  			break;
> +		}
>  		case Opt_gid:
> -			if (match_int(&args[0], &option))
> +		{
> +			char *gidstr = args[0].from;
> +			gid_t gidval;
> +			int rc = kstrtouint(gidstr, 0, &gidval);
> +
> +			if (rc)
>  				return -EINVAL;
> -			gid = make_kgid(current_user_ns(), option);
> +			gid = make_kgid(current_user_ns(), gidval);
>  			if (!gid_valid(gid))
>  				return -EINVAL;
>  			opts->gid = gid;
>  			opts->setgid = 1;
>  			break;
> +		}
>  		case Opt_mode:
>  			if (match_octal(&args[0], &option))
>  				return -EINVAL;
> 

Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t

[Dongsu Park]

On 28.08.2015 15:33, Peter Hurley wrote:
> On 08/18/2015 11:18 AM, Dongsu Park wrote:
> > ---
> >  fs/devpts/inode.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> > index c35ffdc12bba..49272fae40a7 100644
> > --- a/fs/devpts/inode.c
> > +++ b/fs/devpts/inode.c
> > @@ -188,23 +188,35 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
> >  		token = match_token(p, tokens, args);
> >  		switch (token) {
> >  		case Opt_uid:
> > -			if (match_int(&args[0], &option))
> 
> match_int() => make_kuid/kgid is a widespread pattern in filesystems
> for handling uid/gid mount parameters.
> 
> How about adding a for-purpose string-to-uid/gid function, rather than
> open-coding?

Yeah, that sounds like a good idea.
Do you mean probably something like this? (on top of -mm tree)

Thanks,
Dongsu

----

>From ccfa5db398ba5ac31c5e0128e88abca1f6d1e6f5 Mon Sep 17 00:00:00 2001
Message-Id: <ccfa5db398ba5ac31c5e0128e88abca1f6d1e6f5.1440844226.git.dpark@posteo.net>
From: Dongsu Park <dpark@posteo.net>
Date: Sat, 29 Aug 2015 12:35:01 +0200
Subject: [PATCH v3] devpts: allow mounting with uid/gid of uint32_t

To allow devpts to be mounted with options of uid/gid of uint32_t,
we need to make use of general parsing API instead of match_int().
So introduce kstrto{uid,gid}(), wrappers around kstrtouint() as well
as make_k{uid,gid}() calls. And then make devpts parse options only
using kstrto{uid,gid}(). Doing that, mounting devpts with uid or
gid > (2^31 - 1) will work as expected, e.g.:

 # mount -t devpts devpts /tmp/devptsdir -o \
   newinstance,ptmxmode=0666,mode=620,uid=3598450688,gid=3598450693

It was originally by reported on github issue tracker of systemd:
https://github.com/systemd/systemd/issues/956

====
from v2:
 * rebase on top of -mm tree
 * split common parts for parsing uid/gid into kstrto{uid,gid}()
 * fix minor format.
 * continue to use kstrtouint() suggested by Alexey Dobriyan.

from v1: fix patch format correctly

Cc: Alexey Dobriyan <adobriyan@gmail.com>
Reported-by: Alban Crequy <alban@endocode.com>
Suggested-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Dongsu Park <dpark@posteo.net>
---
 fs/devpts/inode.c             | 19 +++++++++----------
 include/linux/parse-integer.h |  4 ++++
 lib/kstrtox.c                 | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index c35ffdc12bba..fbbd71005dcb 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -181,6 +181,7 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 		substring_t args[MAX_OPT_ARGS];
 		int token;
 		int option;
+		int rc;
 
 		if (!*p)
 			continue;
@@ -188,20 +189,18 @@ static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 		token = match_token(p, tokens, args);
 		switch (token) {
 		case Opt_uid:
-			if (match_int(&args[0], &option))
-				return -EINVAL;
-			uid = make_kuid(current_user_ns(), option);
-			if (!uid_valid(uid))
-				return -EINVAL;
+			rc = kstrtouid(args[0].from, &uid);
+			if (rc)
+				return rc;
+
 			opts->uid = uid;
 			opts->setuid = 1;
 			break;
 		case Opt_gid:
-			if (match_int(&args[0], &option))
-				return -EINVAL;
-			gid = make_kgid(current_user_ns(), option);
-			if (!gid_valid(gid))
-				return -EINVAL;
+			rc = kstrtogid(args[0].from, &gid);
+			if (rc)
+				return rc;
+
 			opts->gid = gid;
 			opts->setgid = 1;
 			break;
diff --git a/include/linux/parse-integer.h b/include/linux/parse-integer.h
index ba620cdf3df6..2cdc4f418e00 100644
--- a/include/linux/parse-integer.h
+++ b/include/linux/parse-integer.h
@@ -2,6 +2,7 @@
 #define _PARSE_INTEGER_H
 #include <linux/compiler.h>
 #include <linux/types.h>
+#include <linux/uidgid.h>
 
 /*
  * int parse_integer(const char *s, unsigned int base, T *val);
@@ -155,6 +156,9 @@ static inline int __must_check kstrtos8(const char *s, unsigned int base, s8 *re
 	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
 }
 
+int __must_check kstrtouid(const char *uidstr, kuid_t *kuid);
+int __must_check kstrtogid(const char *gidstr, kgid_t *kgid);
+
 int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
 int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
 int __must_check kstrtoul_from_user(const char __user *s, size_t count, unsigned int base, unsigned long *res);
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 1698b286d954..4c376716a72e 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -17,6 +17,8 @@
 #include <linux/math64.h>
 #include <linux/export.h>
 #include <linux/types.h>
+#include <linux/sched.h>
+#include <linux/cred.h>
 #include <asm/uaccess.h>
 #include "kstrtox.h"
 
@@ -81,6 +83,44 @@ int f(const char __user *s, size_t count, unsigned int base, type *res)	\
 }									\
 EXPORT_SYMBOL(f)
 
+int kstrtouid(const char *uidstr, kuid_t *kuid)
+{
+	uid_t uidval;
+	kuid_t kuidtmp;
+
+	int rc = kstrtouint(uidstr, 0, &uidval);
+
+	if (rc)
+		return rc;
+
+	kuidtmp = make_kuid(current_user_ns(), uidval);
+	if (!uid_valid(kuidtmp))
+		return -EINVAL;
+
+	*kuid = kuidtmp;
+	return 0;
+}
+EXPORT_SYMBOL(kstrtouid);
+
+int kstrtogid(const char *gidstr, kgid_t *kgid)
+{
+	gid_t gidval;
+	kgid_t kgidtmp;
+
+	int rc = kstrtouint(gidstr, 0, &gidval);
+
+	if (rc)
+		return rc;
+
+	kgidtmp = make_kgid(current_user_ns(), gidval);
+	if (!gid_valid(kgidtmp))
+		return -EINVAL;
+
+	*kgid = kgidtmp;
+	return 0;
+}
+EXPORT_SYMBOL(kstrtogid);
+
 kstrto_from_user(kstrtoull_from_user,	kstrtoull,	unsigned long long);
 kstrto_from_user(kstrtoll_from_user,	kstrtoll,	long long);
 kstrto_from_user(kstrtoul_from_user,	kstrtoul,	unsigned long);
-- 
2.1.0

Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t

[Alexey Dobriyan]

On Sat, Aug 29, 2015 at 12:43:04PM +0200, Dongsu Park wrote:
> > How about adding a for-purpose string-to-uid/gid function, rather than
> > open-coding?

>  		case Opt_uid:
> -			if (match_int(&args[0], &option))
> -				return -EINVAL;
> -			uid = make_kuid(current_user_ns(), option);
> -			if (!uid_valid(uid))
> -				return -EINVAL;
> +			rc = kstrtouid(args[0].from, &uid);
> +			if (rc)
> +				return rc;
> +
>  			opts->uid = uid;

if kstrtouid is doing all the work, value should be put directly into
opts->uid avoiding temporary variables.

>  		case Opt_gid:
> -			if (match_int(&args[0], &option))
> -				return -EINVAL;
> -			gid = make_kgid(current_user_ns(), option);
> -			if (!gid_valid(gid))
> -				return -EINVAL;
> +			rc = kstrtogid(args[0].from, &gid);
> +			if (rc)
> +				return rc;

ditto

> +
>  			opts->gid = gid;
>  			opts->setgid = 1;
>  			break;

> --- a/include/linux/parse-integer.h
> +++ b/include/linux/parse-integer.h
> @@ -2,6 +2,7 @@
>  #define _PARSE_INTEGER_H
>  #include <linux/compiler.h>
>  #include <linux/types.h>
> +#include <linux/uidgid.h>

no, just put the prototypes into uidgid.h
This header is supposed to be small and self contained.

> @@ -155,6 +156,9 @@ static inline int __must_check kstrtos8(const char *s, unsigned int base, s8 *re
>  	return parse_integer(s, base | PARSE_INTEGER_NEWLINE, res);
>  }
>  
> +int __must_check kstrtouid(const char *uidstr, kuid_t *kuid);
> +int __must_check kstrtogid(const char *gidstr, kgid_t *kgid);
> +
>  int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
>  int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
>  int __must_check kstrtoul_from_user(const char __user *s, size_t count, unsigned int base, unsigned long *res);

> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -17,6 +17,8 @@
>  #include <linux/math64.h>
>  #include <linux/export.h>
>  #include <linux/types.h>
> +#include <linux/sched.h>
> +#include <linux/cred.h>

This is pretty ridiculous for what is supposed to be bunch  of integer
conversion routines. sched.h? come on!

> @@ -81,6 +83,44 @@ int f(const char __user *s, size_t count, unsigned int base, type *res)	\
>  }									\
>  EXPORT_SYMBOL(f)
>  
> +int kstrtouid(const char *uidstr, kuid_t *kuid)
> +{
> +	uid_t uidval;
> +	kuid_t kuidtmp;
> +
> +	int rc = kstrtouint(uidstr, 0, &uidval);
> +
> +	if (rc)
> +		return rc;

and please follow coding style.

> +
> +	kuidtmp = make_kuid(current_user_ns(), uidval);
> +	if (!uid_valid(kuidtmp))
> +		return -EINVAL;
> +
> +	*kuid = kuidtmp;
> +	return 0;
> +}
> +EXPORT_SYMBOL(kstrtouid);
> +
> +int kstrtogid(const char *gidstr, kgid_t *kgid)
> +{
> +	gid_t gidval;
> +	kgid_t kgidtmp;
> +
> +	int rc = kstrtouint(gidstr, 0, &gidval);
> +
> +	if (rc)
> +		return rc;
> +
> +	kgidtmp = make_kgid(current_user_ns(), gidval);
> +	if (!gid_valid(kgidtmp))
> +		return -EINVAL;
> +
> +	*kgid = kgidtmp;
> +	return 0;
> +}
> +EXPORT_SYMBOL(kstrtogid);