Page MenuHomeFreeBSD

Import _FORTIFY_SOURCE implementation from NetBSD
ClosedPublic

Authored by kevans on Oct 5 2021, 3:47 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 23, 7:59 AM
Unknown Object (File)
Mon, May 20, 9:28 PM
Unknown Object (File)
Sun, May 19, 12:48 AM
Unknown Object (File)
Mon, May 13, 5:25 AM
Unknown Object (File)
Thu, May 2, 5:58 PM
Unknown Object (File)
Thu, May 2, 12:27 PM
Unknown Object (File)
Tue, Apr 30, 1:25 PM
Unknown Object (File)
Tue, Apr 30, 6:17 AM

Details

Summary

This is a mostly-unmodified copy of the various *_chk implementations
and headers from NetBSD, without yet modifying system headers to start
actually including them. A future commit will also apply the needed
bits to fix ssp/unistd.h.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kevans requested review of this revision.Oct 5 2021, 3:47 AM
include/ssp/string.h
100

This is always true...

include/ssp/unistd.h
40

why redirect read(2) and none of the other system calls that write back into user space?

I don't see any others in NetBSD's either. Expanding it is a big ask, so before I really do that as more than maybe a suggestion, I'd like to know the thinking here.

include/ssp/string.h
100

Fair point; I'll hack it back out in the second patch that reworks part of these headers for FreeBSD (with a brief explanation that it's tautological here)

include/ssp/unistd.h
40

I think they more or less tried to stick with the common _FORTIFY_SOURCE implementation (glibc). We can expand it, but IMO let's get to the baseline and define expanding it as future work (explicitly in the commit message) -- the baseline should be 'relatively safe' as folks have been running with glibc's implementation for years as it is.

Since this code goes directly into our libc source, and not into vendor import area, should it follow style(9)? The most systematic thing I noted is return a; instead of return (a); Another thing is the headers inclusion order.

include/ssp/ssp.h
38

I do not think lint is relevant for FreeBSD.

lib/libc/secure/sprintf_chk.c
47

These lint-style comments (as well LINTLIBRARY) are not relevant either.

include/ssp/unistd.h
40

The counter you your argument may also be that fortify_source was done years ago and is showing its age :)....
But what you suggest works for me, so long as we understand that followup work might be good to increase the coverage. Mostly I wanted to understand why it was incomplete.

kevans marked 3 inline comments as done.

Straighten out some style and other bits:

  • clang || GCC >= 4.8 is a given
  • Remove irrelevant LINT/lint bits
  • Shuffle around includes in *_chk.c, alphabetical order with ssp/* headers last
  • Remove spurious prototypes in *_chk.c; these should be provided in ssp/*
  • return x => return (x)
emaste added inline comments.
lib/libssp/Versions.def
2

Is this true?

lib/libssp/Versions.def
2

I added the comment based on the greatest of technicalities, that it appeared in our version of libssp in 13.0-CURRENT (since I added our version of libssp in cd0d51baaa4509a1d); happy enough to revise it, though.

lib/libssp/Versions.def
2

Ok it's fine with me 👍

gbe added inline comments.
lib/libssp/ssp.3
122

Would be good if you could also mention FreeBSD here.

pauamma_gundo.com added inline comments.
lib/libssp/__builtin_object_size.3
101–102

Matters word order. :-)

lib/libssp/ssp.3
121

"in" missing unless .Nx does more than what I remember .Fx doing. (Maybe let upstream know as well?)

kevans marked 2 inline comments as done.
  • Fix word order
  • "in" before .Nx
  • Mention FreeBSD (had GNU version, this version came in in 12.2, enhancements to be in... 14.0)

Manual pages LGTM. Can't attest to the source or the consistency between them.

Why didn't I approve it last No... oh right; didn't know I could or should.

This revision is now accepted and ready to land.Sep 25 2022, 2:21 AM

I think this is fine, modulo notes about its completeness, but I'm OK with that and it's not bad.

In D32306#833118, @imp wrote:

I think this is fine, modulo notes about its completeness, but I'm OK with that and it's not bad.

I'm updating the other reviews to address pending comments; I'd like to land this initial batch that matches what NetBSD has, then I'll start going through other existing headers and see what else we can add.

In D32306#833118, @imp wrote:

I think this is fine, modulo notes about its completeness, but I'm OK with that and it's not bad.

I'm updating the other reviews to address pending comments; I'd like to land this initial batch that matches what NetBSD has, then I'll start going through other existing headers and see what else we can add.

I'm good with that.

So overall this stuff is in principle incompatible with the code like memcmp(*a++, *b++, XXX) ?

include/ssp/ssp.h
83

Can we use gcc ({}) extension there to avoid multiple evaluations for the args?
It still would cause multi-eval when real function is called after __ssp_overlap, though.

lib/libc/secure/Symbol.map
7

This should become 1.8

lib/libc/secure/gets_chk.c
43

extern is not needed

lib/libssp/Versions.def
6

15.0

In D32306#1024959, @kib wrote:

So overall this stuff is in principle incompatible with the code like memcmp(*a++, *b++, XXX) ?

It doesn't need to be, I'll just need to fix the existing macros to avoid multiple evaluations or, perhaps better, go ahead and convert them all to static inlines.

include/ssp/ssp.h
83

We could, though it looks like __ssp_overlap is really only intended to be used in libc so I think I'll move it into an ssp_private.h in libc.

kevans marked 3 inline comments as done.

Fix some nits
Switch to copound statements to avoid double evaluation

This revision now requires review to proceed.Fri, Apr 26, 7:20 AM

Do we need to put SPD tags in the added files?

lib/libssp/__builtin_object_size.3
49

What does it mean for the object to not have a side effect?

lib/libssp/__builtin_object_size.3
49

I'll revise this based on experimentation; the point is, as you probably guessed, that ptr is really just an expression that may or may not have side effects. Consider this example, which must be built with -O1 or higher:

#include <stdio.h>                                                                                            
                                                                                                              
int                                                                                                           
main()                                                                                                        
{                                                                                                             
    char buf[64], *p = buf;                                                                                   
                                                                                                              
    printf("%zu\n", __builtin_object_size(p, 0));                                                             
    printf("%zu\n", __builtin_object_size(++p, 0));                                                           
}

The first printf() gets you 64, the second one gets you (size_t)-1 and p remains unadvanced; this seems like a further argument for the conversion to compound statements, easier to reason about and avoids disabling fortification because of a technicality.

Add SPDX tags

Revise _builtin_object_size(3):

  • Note clang(1) as well, we'll keep GCC enumerated as it originated there and it's worth it to remain clear that this still works with a GCC build.
  • Try to make it clearer that we're talking about the expression that makes up the obj argument.
kib added inline comments.
include/ssp/string.h
38

Not needed

lib/libc/secure/fgets_chk.c
36

We have both __RCSID and the first line. Can we leave only one?

lib/libc/secure/gets_chk.c
36

same

lib/libc/secure/memcpy_chk.c
36

same

lib/libssp/__builtin_object_size.3
52

object referenced by
.Fa ptr
?

This revision is now accepted and ready to land.Sat, Apr 27, 6:08 PM
include/ssp/string.h
57

This doesn't quite work because of, e.g., gems like this from fortune:

460 |                 strcat(strcat(strcpy(tpath, dir), "/"), file);
kevans marked 5 inline comments as done.

Address review feedback:

  • Remove redundant <sys/cdefs.h>
  • Strip redundant __RCSID tags from libc/secure source files
  • Slight re-word in __builtin_object_size(3)
  • Uglify the macros a little further to avoid variable shadowing in nested calls, since we seem to have some of those, using COUNTER (at least supported by gcc/clang). While we're here, a couple of double-evaluations were pointed out and fixed.
This revision now requires review to proceed.Wed, May 1, 4:49 AM
lib/libssp/__builtin_object_size.3
80

This looks stilted to me. Maybe "knows can be pointed"?

lib/libssp/ssp.3
100

Does this apply to clang as well? If so, I'd mention both.

115

Add clang here if added above.

kib added inline comments.
lib/libssp/ssp.3
128

Did it?

This revision is now accepted and ready to land.Wed, May 1, 10:30 PM
kevans added inline comments.
lib/libssp/ssp.3
128

This is referring to https://svnweb.freebsd.org/base/?view=revision&revision=r356356, it just didn't get a man page back then and these symbols aborted.

We wouldn't typically force a rebuild of everything for something like this (use of these additional symbols doesn't cause ABI issues anywhere, no automatic feature detection is needed) , but I suppose I could throw something in src/UPDATING to mention it? It doesn't really need special care, though, just the procedure you'd typically need to follow for living on main...

I could throw something in src/UPDATING to mention it?

That sounds good (and sufficient) to me - just providing a heads-up that some special action may be needed.