linux-kernel - Re: [PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to

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]

Message-ID: <cf6e3669-1644-9611-6acc-781f46dd4f9e@csgroup.eu>
Date:   Mon, 13 Sep 2021 19:01:18 +0200
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>, hch@...radead.org,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH RESEND v3 4/6] signal: Add unsafe_copy_siginfo_to_user32()



Le 13/09/2021 à 17:54, Eric W. Biederman a écrit :
> Christophe Leroy <christophe.leroy@...roup.eu> writes:
> 
>> In the same spirit as commit fb05121fd6a2 ("signal: Add
>> unsafe_get_compat_sigset()"), implement an 'unsafe' version of
>> copy_siginfo_to_user32() in order to use it within user access blocks.
>>
>> To do so, we need inline version of copy_siginfo_to_external32() as we
>> don't want any function call inside user access blocks.
> 
> I don't understand.  What is wrong with?
> 
> #define unsafe_copy_siginfo_to_user32(to, from, label)	do {		\
> 	struct compat_siginfo __user *__ucs_to = to;			\
> 	const struct kernel_siginfo *__ucs_from = from;			\
> 	struct compat_siginfo __ucs_new;				\
> 									\
> 	copy_siginfo_to_external32(&__ucs_new, __ucs_from);		\
> 	unsafe_copy_to_user(__ucs_to, &__ucs_new,			\
> 			    sizeof(struct compat_siginfo), label);	\
> } while (0)

As far as I understood, it is forbidden to call functions within user 
access blocks.

On powerpc it doesn't matter (yet), but as far as I understand x86 as a 
tool called "objtool" to enforce that.


> 
> Your replacement of "memset(to, 0, sizeof(*to))" with
> "struct compat_siginfo __ucs_new = {0}".  is actively unsafe as the
> compiler is free not to initialize any holes in the structure to 0 in
> the later case.

Ah ? I didn't know that.
Maybe we can make as exception for memset(). Or we can hard-code a 
zeroizing loop.

> 
> Is there something about the unsafe macros that I am not aware of that
> makes it improper to actually call C functions?  Is that a requirement
> for the instructions that change the user space access behavior?

See see 
https://lore.kernel.org/lkml/20190318155142.025214872@infradead.org/T/ ?

> 
>  From the looks of this change all that you are doing is making it so
> that all of copy_siginfo_to_external32 is being inlined.  If that is not
> a hard requirement of the instructions it seems like the wrong thing to
> do here. copy_siginfo_to_external32 has not failures so it does not need
> to be inlined so you can jump to the label.

Yes that's what I did, make sure everything is inlined. Or maybe I 
misunderstood something ?

Christophe