Re: [PATCH 1/2] coredump: use from_kuid/kgid_munged when formatting corename

Re: [PATCH 1/2] coredump: use from_kuid/kgid_munged when formatting corename

[Eric W. Biederman]

Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> writes:

> When adding __printf attribute to cn_printf, gcc reports some issues:
>
>   fs/coredump.c:213:5: warning: format '%d' expects argument of type
>   'int', but argument 3 has type 'kuid_t' [-Wformat=]
>        err = cn_printf(cn, "%d", cred->uid);
>        ^
>   fs/coredump.c:217:5: warning: format '%d' expects argument of type
>   'int', but argument 3 has type 'kgid_t' [-Wformat=]
>        err = cn_printf(cn, "%d", cred->gid);
>        ^
>
> These warnings come from the fact that the value of uid/gid needs to be
> extracted from the kuid_t/kgid_t structure before being used as an
> integer.  More precisely, cred->uid and cred->gid need to be converted
> to either user-namespace uid/gid or to init_user_ns uid/gid.
>
> As Documentation/sysctl/kernel.txt does not specify which user namespace
> is used to translate %u and %g in core_pattern, but lowercase %p and %i
> are used to format pid/tid in the current process namespace, it seems
> intuitive that lowercase %u and %g use the current user namespace.

I love the logic of lower vs upper case letters in the selection of how
an identifier should be used.  Unfortunately I can't support it.

Converting to anything other than init_user_ns will actually be an ABI
break.  Which in practice should trump everything else.

Further only the global root user can set this value, which largely
implies that the program setting the core dump patter will expect the
values to be in the initial user namespace.

In practice your patch allows any user to put any uid they want on core
files which seems to make the uid parameter useless.

So for all of the reasons above I think we need to print uids and gids
in the initial user namespace unless explicitly requested to do so.

> While at it, format uid and gid values with %u instead of %d because
> uid_t/__kernel_uid32_t and gid_t/__kernel_gid32_t are unsigned int.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
> ---
>  fs/coredump.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index bbbe139ab280..99c8af640c5a 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -209,11 +209,15 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
>  				break;
>  			/* uid */
>  			case 'u':
> -				err = cn_printf(cn, "%d", cred->uid);
> +				err = cn_printf(cn, "%u",
> +						from_kuid_munged(cred->user_ns,
> +								 cred->uid));
>  				break;
>  			/* gid */
>  			case 'g':
> -				err = cn_printf(cn, "%d", cred->gid);
> +				err = cn_printf(cn, "%u",
> +						from_kgid_munged(cred->user_ns,
> +								 cred->gid));
>  				break;
>  			case 'd':
>  				err = cn_printf(cn, "%d",

Re: [PATCH 1/2] coredump: use from_kuid/kgid_munged when formatting corename

[Nicolas Iooss]

On 05/06/2015 03:13 AM, Eric W. Biederman wrote:
> Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> writes:
> 
>> When adding __printf attribute to cn_printf, gcc reports some issues:
>>
>>   fs/coredump.c:213:5: warning: format '%d' expects argument of type
>>   'int', but argument 3 has type 'kuid_t' [-Wformat=]
>>        err = cn_printf(cn, "%d", cred->uid);
>>        ^
>>   fs/coredump.c:217:5: warning: format '%d' expects argument of type
>>   'int', but argument 3 has type 'kgid_t' [-Wformat=]
>>        err = cn_printf(cn, "%d", cred->gid);
>>        ^
>>
>> These warnings come from the fact that the value of uid/gid needs to be
>> extracted from the kuid_t/kgid_t structure before being used as an
>> integer.  More precisely, cred->uid and cred->gid need to be converted
>> to either user-namespace uid/gid or to init_user_ns uid/gid.
>>
>> As Documentation/sysctl/kernel.txt does not specify which user namespace
>> is used to translate %u and %g in core_pattern, but lowercase %p and %i
>> are used to format pid/tid in the current process namespace, it seems
>> intuitive that lowercase %u and %g use the current user namespace.
> 
> I love the logic of lower vs upper case letters in the selection of how
> an identifier should be used.  Unfortunately I can't support it.
> 
> Converting to anything other than init_user_ns will actually be an ABI
> break.  Which in practice should trump everything else.

I agree.  Initially I thought that my patch introduced only a minor
change to make the ABI more consistent, but if you think this change is
actually large enough to be considered a "not-wanted ABI break", I will
follow your decision.

> Further only the global root user can set this value, which largely
> implies that the program setting the core dump patter will expect the
> values to be in the initial user namespace.

Ok, I missed this.  The main source of my confusion is that the coredump
uses the current mount namespace to create files (I tested using a
Docker container with core_pattern set to something in /tmp) and the
global process/mount namespaces when using pipes (tested with
systemd-coredump).

> In practice your patch allows any user to put any uid they want on core
> files which seems to make the uid parameter useless.
> 
> So for all of the reasons above I think we need to print uids and gids
> in the initial user namespace unless explicitly requested to do so.

So be it.  I'll update my patches and send them again.  In order to make
things clearer, I also would like to modify the documentation of %u and
%g, with something like:

--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -197,8 +197,8 @@
 	%P	global pid (init PID namespace)
 	%i	tid
 	%I	global tid (init PID namespace)
-	%u	uid
-	%g	gid
+	%u	uid (in init user namespace)
+	%g	gid (in init user namespace)
 	%d	dump mode, matches PR_SET_DUMPABLE and
 		/proc/sys/fs/suid_dumpable
 	%s	signal number

Would such a change be accepted, and if yes is it better to put it in
its own commit or in the same as the one modifying the code?

Thanks,

Nicolas

[PATCH v2 1/2] coredump: use from_kuid/kgid when formatting corename

[Nicolas Iooss]

When adding __printf attribute to cn_printf, gcc reports some issues:

  fs/coredump.c:213:5: warning: format '%d' expects argument of type
  'int', but argument 3 has type 'kuid_t' [-Wformat=]
       err = cn_printf(cn, "%d", cred->uid);
       ^
  fs/coredump.c:217:5: warning: format '%d' expects argument of type
  'int', but argument 3 has type 'kgid_t' [-Wformat=]
       err = cn_printf(cn, "%d", cred->gid);
       ^

These warnings come from the fact that the value of uid/gid needs to be
extracted from the kuid_t/kgid_t structure before being used as an
integer.  More precisely, cred->uid and cred->gid need to be converted
to either user-namespace uid/gid or to init_user_ns uid/gid.

Use init_user_ns in order not to break existing ABI, and document this
in Documentation/sysctl/kernel.txt.

While at it, format uid and gid values with %u instead of %d because
uid_t/__kernel_uid32_t and gid_t/__kernel_gid32_t are unsigned int.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
---
 Documentation/sysctl/kernel.txt | 4 ++--
 fs/coredump.c                   | 8 ++++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index c831001c45f1..e1913f78f21e 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -197,8 +197,8 @@ core_pattern is used to specify a core dumpfile pattern name.
 	%P	global pid (init PID namespace)
 	%i	tid
 	%I	global tid (init PID namespace)
-	%u	uid
-	%g	gid
+	%u	uid (in initial user namespace)
+	%g	gid (in initial user namespace)
 	%d	dump mode, matches PR_SET_DUMPABLE and
 		/proc/sys/fs/suid_dumpable
 	%s	signal number
diff --git a/fs/coredump.c b/fs/coredump.c
index bbbe139ab280..833a57bc856c 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -209,11 +209,15 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 				break;
 			/* uid */
 			case 'u':
-				err = cn_printf(cn, "%d", cred->uid);
+				err = cn_printf(cn, "%u",
+						from_kuid(&init_user_ns,
+							  cred->uid));
 				break;
 			/* gid */
 			case 'g':
-				err = cn_printf(cn, "%d", cred->gid);
+				err = cn_printf(cn, "%u",
+						from_kgid(&init_user_ns,
+							  cred->gid));
 				break;
 			case 'd':
 				err = cn_printf(cn, "%d",
-- 
2.4.0

[PATCH v2 2/2] coredump: add __printf attribute to cn_*printf functions

[Nicolas Iooss]

This allows detecting improper format string at build time, like:

  fs/coredump.c:225:5: warning: format '%ld' expects argument of type
  'long int', but argument 3 has type 'int' [-Wformat=]
       err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
       ^

As si_signo is always an int, the format should be %d here.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
---
 fs/coredump.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index 833a57bc856c..e52e0064feac 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -70,7 +70,8 @@ static int expand_corename(struct core_name *cn, int size)
 	return 0;
 }
 
-static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
+static __printf(2, 0) int cn_vprintf(struct core_name *cn, const char *fmt,
+				     va_list arg)
 {
 	int free, need;
 	va_list arg_copy;
@@ -93,7 +94,7 @@ again:
 	return -ENOMEM;
 }
 
-static int cn_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3) int cn_printf(struct core_name *cn, const char *fmt, ...)
 {
 	va_list arg;
 	int ret;
@@ -105,7 +106,8 @@ static int cn_printf(struct core_name *cn, const char *fmt, ...)
 	return ret;
 }
 
-static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
+static __printf(2, 3)
+int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
 {
 	int cur = cn->used;
 	va_list arg;
@@ -225,7 +227,8 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
 				break;
 			/* signal that caused the coredump */
 			case 's':
-				err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
+				err = cn_printf(cn, "%d",
+						cprm->siginfo->si_signo);
 				break;
 			/* UNIX time of coredump */
 			case 't': {
-- 
2.4.0

Re: [PATCH v2 1/2] coredump: use from_kuid/kgid when formatting corename

[Eric W. Biederman]

Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> writes:

> When adding __printf attribute to cn_printf, gcc reports some issues:
>
>   fs/coredump.c:213:5: warning: format '%d' expects argument of type
>   'int', but argument 3 has type 'kuid_t' [-Wformat=]
>        err = cn_printf(cn, "%d", cred->uid);
>        ^
>   fs/coredump.c:217:5: warning: format '%d' expects argument of type
>   'int', but argument 3 has type 'kgid_t' [-Wformat=]
>        err = cn_printf(cn, "%d", cred->gid);
>        ^
>
> These warnings come from the fact that the value of uid/gid needs to be
> extracted from the kuid_t/kgid_t structure before being used as an
> integer.  More precisely, cred->uid and cred->gid need to be converted
> to either user-namespace uid/gid or to init_user_ns uid/gid.
>
> Use init_user_ns in order not to break existing ABI, and document this
> in Documentation/sysctl/kernel.txt.
>
> While at it, format uid and gid values with %u instead of %d because
> uid_t/__kernel_uid32_t and gid_t/__kernel_gid32_t are unsigned int.

Acked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
> ---
>  Documentation/sysctl/kernel.txt | 4 ++--
>  fs/coredump.c                   | 8 ++++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index c831001c45f1..e1913f78f21e 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -197,8 +197,8 @@ core_pattern is used to specify a core dumpfile pattern name.
>  	%P	global pid (init PID namespace)
>  	%i	tid
>  	%I	global tid (init PID namespace)
> -	%u	uid
> -	%g	gid
> +	%u	uid (in initial user namespace)
> +	%g	gid (in initial user namespace)
>  	%d	dump mode, matches PR_SET_DUMPABLE and
>  		/proc/sys/fs/suid_dumpable
>  	%s	signal number
> diff --git a/fs/coredump.c b/fs/coredump.c
> index bbbe139ab280..833a57bc856c 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -209,11 +209,15 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
>  				break;
>  			/* uid */
>  			case 'u':
> -				err = cn_printf(cn, "%d", cred->uid);
> +				err = cn_printf(cn, "%u",
> +						from_kuid(&init_user_ns,
> +							  cred->uid));
>  				break;
>  			/* gid */
>  			case 'g':
> -				err = cn_printf(cn, "%d", cred->gid);
> +				err = cn_printf(cn, "%u",
> +						from_kgid(&init_user_ns,
> +							  cred->gid));
>  				break;
>  			case 'd':
>  				err = cn_printf(cn, "%d",

Re: [PATCH v2 2/2] coredump: add __printf attribute to cn_*printf functions

[Eric W. Biederman]

Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> writes:

> This allows detecting improper format string at build time, like:
>
>   fs/coredump.c:225:5: warning: format '%ld' expects argument of type
>   'long int', but argument 3 has type 'int' [-Wformat=]
>        err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
>        ^
>
> As si_signo is always an int, the format should be %d here.

Acked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
> ---
>  fs/coredump.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 833a57bc856c..e52e0064feac 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -70,7 +70,8 @@ static int expand_corename(struct core_name *cn, int size)
>  	return 0;
>  }
>  
> -static int cn_vprintf(struct core_name *cn, const char *fmt, va_list arg)
> +static __printf(2, 0) int cn_vprintf(struct core_name *cn, const char *fmt,
> +				     va_list arg)
>  {
>  	int free, need;
>  	va_list arg_copy;
> @@ -93,7 +94,7 @@ again:
>  	return -ENOMEM;
>  }
>  
> -static int cn_printf(struct core_name *cn, const char *fmt, ...)
> +static __printf(2, 3) int cn_printf(struct core_name *cn, const char *fmt, ...)
>  {
>  	va_list arg;
>  	int ret;
> @@ -105,7 +106,8 @@ static int cn_printf(struct core_name *cn, const char *fmt, ...)
>  	return ret;
>  }
>  
> -static int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
> +static __printf(2, 3)
> +int cn_esc_printf(struct core_name *cn, const char *fmt, ...)
>  {
>  	int cur = cn->used;
>  	va_list arg;
> @@ -225,7 +227,8 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm)
>  				break;
>  			/* signal that caused the coredump */
>  			case 's':
> -				err = cn_printf(cn, "%ld", cprm->siginfo->si_signo);
> +				err = cn_printf(cn, "%d",
> +						cprm->siginfo->si_signo);
>  				break;
>  			/* UNIX time of coredump */
>  			case 't': {