False positives in nolibc check

False positives in nolibc check

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]

Hi,
The nolibc build is enabled on x86-64, i686, and aarch64 since commit
bfb432f4cce5 ("configure: Always enable `CONFIG_NOLIBC` if the arch is
supported").

When I build the liburing 2.4 rpm package for Fedora i686, nolibc is
enabled but the following compilation error occurs:

  gcc -fPIC -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -m32 -march=i686 -mtune=generic -msse2 -mfpmath=sse -mstackrealign -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection  -Wno-unused-parameter -DLIBURING_INTERNAL  -nostdlib -nodefaultlibs -ffreestanding -fno-builtin -shared -Wl,--version-script=liburing.map -Wl,-soname=liburing.so.2 -o liburing.so.2.4 setup.os queue.os register.os syscall.os version.os nolibc.os -Wl,-z,relro -Wl,--as-needed  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -Wl,--build-id=sha1 -specs=/usr/lib/rpm/redhat/redhat-package-notes  -nostdlib -nodefaultlibs
  make[1]: Leaving directory '/builddir/build/BUILD/liburing-2.4/src'
  /usr/bin/ld: /tmp/cca16k90.ltrans0.ltrans.o: in function `__io_uring_submit':
  /builddir/build/BUILD/liburing-2.4/src/queue.c:388: undefined reference to `__stack_chk_fail_local'

This is caused by the stack protector compiler options, which depend on
the libc __stack_chk_fail_local symbol.

The compile_prog check in ./configure should use the final
CFLAGS/LDFLAGS (including -ffreestanding) that liburing is compiled with
to avoid false positives. That way it can detect that nolibc won't work
with these compiler options and fall back to using libc.

In general, I'm concerned that nolibc is fragile because the toolchain
and libc sometimes have dependencies that are activated by certain
compiler options. Some users will want libc and others will not. Maybe
make it an explicit option instead of probing?

I've included a downstream patch in the Fedora package that disables
nolibc for the time being.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: False positives in nolibc check

[Alviro Iskandar Setiawan]

Hello Stefan,

On Tue, Jun 20, 2023 at 8:32 PM Stefan Hajnoczi wrote:
> This is caused by the stack protector compiler options, which depend on
> the libc __stack_chk_fail_local symbol.

liburing itself explicitly disables the stack protector, even when
compiled with libc. You customize the build and use something that
needs libc (stack protector). So I would say liburing upstream has
taken care of this problem in the normal build.

> The compile_prog check in ./configure should use the final
> CFLAGS/LDFLAGS (including -ffreestanding) that liburing is compiled with
> to avoid false positives. That way it can detect that nolibc won't work
> with these compiler options and fall back to using libc.
>
> In general, I'm concerned that nolibc is fragile because the toolchain
> and libc sometimes have dependencies that are activated by certain
> compiler options. Some users will want libc and others will not. Maybe
> make it an explicit option instead of probing?

I'm not sure it's worth using libc in liburing (x86(-64) and aarch64)
just to activate the stack protector. Do you have other convincing use
cases where libc is strictly needed on architectures that support
liburing nolibc?

I think using stack protector for liburing is just too overkill, but I
may be wrong, please tell me a good reason for using it in liburing.

I admit that nolibc brings problems. For example, the memset() issue
on aarch64 recently (it's fixed). If you have similar problems, please
tell. We probably should consider bringing back the "--nolibc" option
in the "./configure" file?

> I've included a downstream patch in the Fedora package that disables
> nolibc for the time being.

Thanks for maintaining the package. I appreciate it.

-- Viro

Re: False positives in nolibc check

[Ammar Faizi]

On Tue, Jun 20, 2023 at 03:31:52PM +0200, Stefan Hajnoczi wrote:
> This is caused by the stack protector compiler options, which depend on
> the libc __stack_chk_fail_local symbol.

Guillem fixed it last week. Does this commit fix the stack protector
problem? https://github.com/axboe/liburing/commit/319f4be8bd049055c333185928758d0fb445fc43

> In general, I'm concerned that nolibc is fragile because the toolchain
> and libc sometimes have dependencies that are activated by certain
> compiler options. Some users will want libc and others will not. Maybe
> make it an explicit option instead of probing?

I made nolibc always enabled because I don't see the need of using libc
in liburing. If we have a real reason of using libc, maybe adding
'--use-libc' is better than bringing back '--nolibc'. 

I agree with what Alviro said that using stack protector in liburing is
too overkill. That's why I see no reason for the upstream to support it.

Can you mention other dependencies that do need libc? That information
would be useful to consider bringing back libc to liburing.

Regards,
-- 
Ammar Faizi

Re: False positives in nolibc check

[Alviro Iskandar Setiawan]

On Tue, Jun 20, 2023 at 10:49 PM Ammar Faizi <ammarfaizi2@gnuweeb.org> wrote:
> Can you mention other dependencies that do need libc? That information
> would be useful to consider bringing back libc to liburing.

The recent memset() problem is another example of it. It seems that
the compiler on aarch64 replaces a zeroing struct operation with a
call to memset(). I usually see the same memset() insertion on x86-64
too. I thought -ffreestanding will make the compiler not generate a
memset() call when zeroing a struct or array. Not sure about it.

-- Viro

Re: False positives in nolibc check

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3184 bytes --]

On Tue, Jun 20, 2023 at 09:39:46PM +0700, Alviro Iskandar Setiawan wrote:
> Hello Stefan,
> 
> On Tue, Jun 20, 2023 at 8:32 PM Stefan Hajnoczi wrote:
> > This is caused by the stack protector compiler options, which depend on
> > the libc __stack_chk_fail_local symbol.
> 
> liburing itself explicitly disables the stack protector, even when
> compiled with libc. You customize the build and use something that
> needs libc (stack protector). So I would say liburing upstream has
> taken care of this problem in the normal build.

Do you mean this:

src/Makefile:CFLAGS ?= -g -O3 -Wall -Wextra -fno-stack-protector

?

CFLAGS is set in the rpmbuild environment and therefore the ?= operator
has no effect. Here is the build log:
https://kojipkgs.fedoraproject.org//packages/liburing/2.4/2.fc38/data/logs/i686/build.log

If -fno-stack-protector is required, then the build system should fail
and let the user know that an unsupported flag was detected instead of
silently allowing an unsupported flag.

> 
> > The compile_prog check in ./configure should use the final
> > CFLAGS/LDFLAGS (including -ffreestanding) that liburing is compiled with
> > to avoid false positives. That way it can detect that nolibc won't work
> > with these compiler options and fall back to using libc.
> >
> > In general, I'm concerned that nolibc is fragile because the toolchain
> > and libc sometimes have dependencies that are activated by certain
> > compiler options. Some users will want libc and others will not. Maybe
> > make it an explicit option instead of probing?
> 
> I'm not sure it's worth using libc in liburing (x86(-64) and aarch64)
> just to activate the stack protector. Do you have other convincing use
> cases where libc is strictly needed on architectures that support
> liburing nolibc?

libc isn't strictly needed for stack protector. liburing could go
further down the path of duplicating libc symbols and implement
__stack_chk_fail_local itself.

However, I don't understand the reason for nolibc in the first place. Is
it because liburing is used by non-C languages where libc conflicts with
their runtime environment/library? I'm surprised by that since
FFI-friendly languages should be used to the presence of libc. Also, I'm
not sure why liburing.so should be nolibc for this use case, since there
is liburing-ffi.so specifically for FFI users.

> I think using stack protector for liburing is just too overkill, but I
> may be wrong, please tell me a good reason for using it in liburing.

I think that should be left up to packagers. Some distributions may want
to compile with a standard set of hardening options. I'm not sure what
the justification for making an exception for liburing should be?
Security folks won't be happy :).

> I admit that nolibc brings problems. For example, the memset() issue
> on aarch64 recently (it's fixed). If you have similar problems, please
> tell. We probably should consider bringing back the "--nolibc" option
> in the "./configure" file?

I don't have a strong opinion on the solution here, just that liburing
should compile successfully.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: False positives in nolibc check

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2068 bytes --]

On Tue, Jun 20, 2023 at 10:49:08PM +0700, Ammar Faizi wrote:
> On Tue, Jun 20, 2023 at 03:31:52PM +0200, Stefan Hajnoczi wrote:
> > This is caused by the stack protector compiler options, which depend on
> > the libc __stack_chk_fail_local symbol.
> 
> Guillem fixed it last week. Does this commit fix the stack protector
> problem? https://github.com/axboe/liburing/commit/319f4be8bd049055c333185928758d0fb445fc43
> 
> > In general, I'm concerned that nolibc is fragile because the toolchain
> > and libc sometimes have dependencies that are activated by certain
> > compiler options. Some users will want libc and others will not. Maybe
> > make it an explicit option instead of probing?
> 
> I made nolibc always enabled because I don't see the need of using libc
> in liburing. If we have a real reason of using libc, maybe adding
> '--use-libc' is better than bringing back '--nolibc'. 
> 
> I agree with what Alviro said that using stack protector in liburing is
> too overkill. That's why I see no reason for the upstream to support it.
> 
> Can you mention other dependencies that do need libc? That information
> would be useful to consider bringing back libc to liburing.

I don't know which features require the toolchain and libc to cooperate.
I guess Thread Local Storage won't work and helper functions that
compilers emit (like the memset example that Alviro gave).

Disabling hardening because it requires work to support it in a nolibc
world seems dubious to me. I don't think it's a good idea for io_uring
to lower security because that hurts its image and reduces adoption.
Especially right now, when the security of io_uring is being scrutinized
(https://security.googleblog.com/2023/06/learnings-from-kctf-vrps-42-linux.html).

While I'm sharing these opinions with you, I understand that some people
want nolibc and are fine with disabling the stack protector. The main
thing I would like is for liburing to compile or fail with a clear error
message instead of breaking somewhere during the build.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: False positives in nolibc check

[Ammar Faizi]

On Wed, Jun 21, 2023 at 12:04:47PM +0200, Stefan Hajnoczi wrote:
> I don't know which features require the toolchain and libc to cooperate.
> I guess Thread Local Storage won't work and helper functions that
> compilers emit (like the memset example that Alviro gave).

Yeah, thread local storage won't work. But the point of my question is
about liburing. So I expect the answer that's relevant to liburing.

I mean, you can still use libc and TLS in your app even though the
liburing.so and liburing.a are nolibc.

> Disabling hardening because it requires work to support it in a nolibc
> world seems dubious to me. I don't think it's a good idea for io_uring
> to lower security because that hurts its image and reduces adoption.
> Especially right now, when the security of io_uring is being scrutinized
> (https://security.googleblog.com/2023/06/learnings-from-kctf-vrps-42-linux.html).
> 
> While I'm sharing these opinions with you, I understand that some people
> want nolibc and are fine with disabling the stack protector. The main
> thing I would like is for liburing to compile or fail with a clear error
> message instead of breaking somewhere during the build.

Right, my mistake. I think it's fixed in upstream by commit:

   319f4be8bd049055c333185928758d0fb445fc43 ("build: Disable stack protector unconditionally")

Please give it a whirl. I apologize for breaking the Fedora build.

Regards,
-- 
Ammar Faizi

Re: False positives in nolibc check

[Guillem Jover]

Hi!

On Wed, 2023-06-21 at 17:19:18 +0700, Ammar Faizi wrote:
> On Wed, Jun 21, 2023 at 12:04:47PM +0200, Stefan Hajnoczi wrote:
> > I don't know which features require the toolchain and libc to cooperate.
> > I guess Thread Local Storage won't work and helper functions that
> > compilers emit (like the memset example that Alviro gave).
> 
> Yeah, thread local storage won't work. But the point of my question is
> about liburing. So I expect the answer that's relevant to liburing.
> 
> I mean, you can still use libc and TLS in your app even though the
> liburing.so and liburing.a are nolibc.

> > Disabling hardening because it requires work to support it in a nolibc
> > world seems dubious to me. I don't think it's a good idea for io_uring
> > to lower security because that hurts its image and reduces adoption.
> > Especially right now, when the security of io_uring is being scrutinized
> > (https://security.googleblog.com/2023/06/learnings-from-kctf-vrps-42-linux.html).
> > 
> > While I'm sharing these opinions with you, I understand that some people
> > want nolibc and are fine with disabling the stack protector. The main
> > thing I would like is for liburing to compile or fail with a clear error
> > message instead of breaking somewhere during the build.
> 
> Right, my mistake. I think it's fixed in upstream by commit:
> 
>    319f4be8bd049055c333185928758d0fb445fc43 ("build: Disable stack protector unconditionally")

While I sent that to make it build again, I have to say when I was
preparing the new liburing upload for Debian I hesitated to simply
disable nolibc support for all arches there. Went for now with this
because it is what is supported upstream and seemed like the smaller
delta for now, and going through all functions it seemed "safe", but
I might revisit this TBH.

We have been through this already with libaio, where going through the
nolibc model also caused problems, see:

  https://pagure.io/libaio/c/672eaebd131c789a528e3a9cd089b4b69a82012b

So, I also think I'd appreciate a --use-libc mode (or similar) which I'd
probably consider enabling for Debian. OTOH, I've no idea how much impact
that would cause to performance? Do any of you have numbers?

Thanks,
Guillem

Re: False positives in nolibc check

[Ammar Faizi]

On Wed, Jun 21, 2023 at 01:51:56PM +0200, Guillem Jover wrote:
> So, I also think I'd appreciate a --use-libc mode (or similar) which I'd
> probably consider enabling for Debian.

I'll send a patch to add that mode for review tomorrow morning.

> OTOH, I've no idea how much impact that would cause to performance? Do
> any of you have numbers?

The only real *hot path* that depended on a libc function was
io_uring_submit(). It used syscall(2) function.

But now even when compiled with libc, we no longer use syscall(2),
instead we use inline assembly to invoke syscall. So there is nothing to
worry about performance here.

Side note:
liburing still uses syscall(2) on architectures other than x86, x86-64
and aarch64.

-- 
Ammar Faizi

Re: False positives in nolibc check

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

On Wed, Jun 21, 2023 at 05:19:18PM +0700, Ammar Faizi wrote:
> On Wed, Jun 21, 2023 at 12:04:47PM +0200, Stefan Hajnoczi wrote:
> > I don't know which features require the toolchain and libc to cooperate.
> > I guess Thread Local Storage won't work and helper functions that
> > compilers emit (like the memset example that Alviro gave).
> 
> Yeah, thread local storage won't work. But the point of my question is
> about liburing. So I expect the answer that's relevant to liburing.
> 
> I mean, you can still use libc and TLS in your app even though the
> liburing.so and liburing.a are nolibc.
> 
> > Disabling hardening because it requires work to support it in a nolibc
> > world seems dubious to me. I don't think it's a good idea for io_uring
> > to lower security because that hurts its image and reduces adoption.
> > Especially right now, when the security of io_uring is being scrutinized
> > (https://security.googleblog.com/2023/06/learnings-from-kctf-vrps-42-linux.html).
> > 
> > While I'm sharing these opinions with you, I understand that some people
> > want nolibc and are fine with disabling the stack protector. The main
> > thing I would like is for liburing to compile or fail with a clear error
> > message instead of breaking somewhere during the build.
> 
> Right, my mistake. I think it's fixed in upstream by commit:
> 
>    319f4be8bd049055c333185928758d0fb445fc43 ("build: Disable stack protector unconditionally")
> 
> Please give it a whirl. I apologize for breaking the Fedora build.

No need to apologize! Thanks for taking a look.

I ran an rpmbuild with liburing.git/master and it succeeds now.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]