Re: git: cf8e5289a110 - main - include: ssp: round out fortification of current set of headers

On 11/11/24 13:43, Kyle Evans wrote:
> On 11/11/24 15:13, John Baldwin wrote:
>> On 7/13/24 22:23, Kyle Evans wrote:
>>> The branch main has been updated by kevans:
>>>
>>> URL:
>>> https://cgit.FreeBSD.org/src/commit/?id=cf8e5289a110954600f135024d1515a77d0ae34d
>>>
>>> commit cf8e5289a110954600f135024d1515a77d0ae34d
>>> Author:     Kyle Evans <kevans@FreeBSD.org>
>>> AuthorDate: 2024-07-13 05:16:10 +0000
>>> Commit:     Kyle Evans <kevans@FreeBSD.org>
>>> CommitDate: 2024-07-13 05:16:24 +0000
>>>
>>>       include: ssp: round out fortification of current set of headers
>>>       ssp/ssp.h needed some improvements:
>>>        - `len` isn't always a size_t, it may need casted
>>>        - In some cases we may want to use a len that isn't specified as a
>>>           parameter (e.g., L_ctermid), so __ssp_redirect() should be more
>>>           flexible.
>>>        - In other cases we may want additional checking, so pull all of
>>> the
>>>           declaration bits out of __ssp_redirect_raw() so that some
>>> functions
>>>           can implement the body themselves.
>>>       strlcat/strlcpy should be the last of the fortified functions
>>> that get
>>>       their own __*_chk symbols, and these cases are only done to be
>>>       consistent with the rest of the str*() set.
>>>       Reviewed by:    markj
>>>       Sponsored by:   Klara, Inc.
>>>       Sponsored by:   Stormshield
>>>       Differential Revision:  https://reviews.freebsd.org/D45679
>>
>> For the change in <sys/libkern.h>, is the intention for <ssp/ssp.h> to only
>> be included in userspace binaries that use this header for some reason?  As
>> it is, there are a handful of files compiled in the kernel that use remove
>> -nostdinc from CFLAGS to access intrinsic headers for things like crypto
>> instructions and those files end up including all of <ssp/ssp.h> in the
>> kernel, e.g. this from armv8crypto:
>>
>> # Remove -nostdinc so we can get the intrinsics.
>> armv8_crypto_wrap.o: armv8_crypto_wrap.c
>>       ${CC} -c ${CFLAGS:C/^-O2$/-O3/:N-nostdinc:N-mgeneral-regs-only} \
>>           -I${SRCTOP}/sys/crypto/armv8 \
>>           ${WERROR} ${PROF} \
>>            -march=armv8-a+crypto ${.IMPSRC}
>>       ${CTFCONVERT_CMD}
>>
>> For CHERI this breaks in an obscure way (which is why I discovered this),
>> but I'm curious what the intention is?  Should the kernel always be
>> using the fallback definition of __ssp_real?
>>
> 
> I think this was meant to address some cross-build issue somewhere in
> our bootstrapped stuff; if it was a kernel vs. userspace thing I don't
> see why I wouldn't have used _KERNEL instead.  The kernel should always
> use the stub that just defines it back to the name passed in, though, so
> we could definitely add a '&& !defined(_KERNEL)' there.

Ok, thanks, that's what I've done locally.  I'll throw it into a review in
a bit.

-- 
John Baldwin