On 05/06/2015 03:13 AM, Eric W. Biederman wrote:
> Nicolas Iooss <nicolas.iooss_li...@m4x.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
