Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1215530 > unrolled thread

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

Started byPeter Hurley <peter@hurleysoftware.com>
First post2015-08-28 21:40 +0200
Last post2015-08-29 23:50 +0200
Articles 3 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t Peter Hurley <peter@hurleysoftware.com> - 2015-08-28 21:40 +0200
    Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t Dongsu Park <dpark@posteo.net> - 2015-08-29 13:00 +0200
      Re: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t Alexey Dobriyan <adobriyan@gmail.com> - 2015-08-29 23:50 +0200

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

FromPeter Hurley <peter@hurleysoftware.com>
Date2015-08-28 21:40 +0200
SubjectRe: [PATCH v2] devpts: allow mounting with uid/gid of uint32_t
Message-ID<q2xFf-4lL-1@gated-at.bofh.it>
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;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1215763

FromDongsu Park <dpark@posteo.net>
Date2015-08-29 13:00 +0200
Message-ID<q2M1z-7ZO-9@gated-at.bofh.it>
In reply to#1215530
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1215848

FromAlexey Dobriyan <adobriyan@gmail.com>
Date2015-08-29 23:50 +0200
Message-ID<q2WaB-5W1-1@gated-at.bofh.it>
In reply to#1215763
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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web