linux-kernel - Re: [PATCH 0/1] tools/nolibc/string: export strlen()

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]

Date: Sat, 27 Jan 2024 18:28:25 +0100
From: Rodrigo Campos <rodrigo@...g.com.ar>
To: Willy Tarreau <w@....eu>, Thomas Weißschuh
 <linux@...ssschuh.net>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/1] tools/nolibc/string: export strlen()

On 1/27/24 17:24, Willy Tarreau wrote:
> On Sat, Jan 27, 2024 at 03:53:32PM +0100, Thomas Weißschuh wrote:
>> On 2024-01-26 15:24:10+0100, Rodrigo Campos wrote:
>>> Please note that this code is not using strlen() and strlcat() doesn't seems to use it either.
>>
>> I think the comment in strlen() explains it:
>>
>>      Note that gcc12 recognizes an strlen() pattern and replaces it with a
>>      jump to strlen().
>>
>> strlcat() indeed contains this pattern.
>>
>> I was able to fix the issue by replacing the open-coded strlen() in
>> strlcat() with a call to the function and that also fixed the issue.
>>
>> It seems nicer to me as a fix, on the other hand the change to a weak
>> definition will also catch other instances of the issue.
>> Maybe we do both.
> 
> Yes, once we have the proof that the compiler may produce such a call, it
> can also happen in whatever user code so we need to export the function,
> there's no other solution.

Makes sense, thanks!

> 
>>> First I noted that removing the attribute unused in strlen(), the compilation worked fine. And then
>>> I noticied that other functions had the attribute weak, a custom section and export the function.
>>>
>>> In particular, what happens here seems to be the same as in commit "tools/nolibc/string: export memset() and
>>> memmove()" (8d304a374023), as removing the -Os or adding the -ffreestanding seem to fix the issue.
>>> So, I did the same as that commit, for strlen().
>>>
>>> However, I'm not 100% confident on how to check that this is done by the compiler to later replace
>>> it and provide a builtin. I'm not sure how that was verified for commit 8d304a374023, but if you let
>>> me know, I can verify it too.
>>>
>>> What do you think?
>>
>> Personally I don't know how it was verified, we'll have to wait for
>> Willy on that.
> 
> Oh it's very simple, just build a small code that doesn't contain any
> such explicit nor implicit call and check that it doesn't contain the
> function.
> 
> E.g >
>    $ printf "int main(void) { }\n" | gcc -nostdlib -static -Isysroot/x86/include -include nolibc.h -Os -Wl,--gc-sections -xc -
>    $ nm --size a.out

Oh, cool. I can confirm that gcc does indeed add the strlen call
(note I had to remove the "--size" param to nm, as the symbol is 
undefined and not shown otherwise)

I wonder if there is an easy way to check for which functions gcc/clang 
do this...

>>> As a side note, it seems strlcat()/strlcpy() fail to set the terminating null byte on some cases,
> 
> Indeed I've just checked and you're right, that defeats their purpose!

Cool.

>>> Let me know if you think it is worth adding some _simple_ patches (I don't think it is worth fixing
>>> all the cases, the code is to fix all of the cases is probably not nice and not worth it).
>>
>> Souns reasonable to me to fix the return values.
>> And get some tests for it.
> 
> Seconded!

Thanks, I'll see how to improve that too :)



Best,
Rodrigo