implement internal safe_strcpy to replace the strncpy

[arnopo]

Compiler complain aboiut the use of strncpy that is not safe.

Already 3 PRs propose a fix of the issue:

• openamp: replace all strncpy to strlcpy #602
• lib: fix usage of strncpy #618
• remoteproc.c: fix compile error #617

strlcpy seems to have been recently integrated into glibc, but it appears to only prevent overflow in the destination buffer without ensuring that we do not perform a read out of the source buffer.

[arnopo]

@wyr-7, @tomi-font, Please, Could you check that this PR fits your needs and, if so, review the PR?

[laxiLang]

I got such AddressSanitizer error from the "string.c":

==794327==ERROR: AddressSanitizer: global-buffer-overflow on address 0x08202c63 at pc 0xed441cde bp 0xe71feba8 sp 0xe71fe778
READ of size 1 at 0x08202c63 thread T2
    #0 0xed441cdd in __interceptor_strlen ../../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:389
    #1 0x80bbf1b in strlcpy ../../../../../../../modules/lib/open-amp/open-amp/lib/utils/string.c:52
0x08202c63 is located 0 bytes to the right of global variable '*.LC24' defined in '../../../../../../../modules/lib/open-amp/open-amp/lib/rpmsg/rpmsg_virtio.c' (0x8202c60) of size 3
  '*.LC24' is ascii string 'NS'
SUMMARY: AddressSanitizer: global-buffer-overflow ../../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:389 in __interceptor_strlen

It can resolved with the changes in https://github.com/zephyrproject-rtos/open-amp/pull/23

[arnopo]

I got such AddressSanitizer error from the "string.c":

==794327==ERROR: AddressSanitizer: global-buffer-overflow on address 0x08202c63 at pc 0xed441cde bp 0xe71feba8 sp 0xe71fe778
READ of size 1 at 0x08202c63 thread T2
    #0 0xed441cdd in __interceptor_strlen ../../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:389
    #1 0x80bbf1b in strlcpy ../../../../../../../modules/lib/open-amp/open-amp/lib/utils/string.c:52
0x08202c63 is located 0 bytes to the right of global variable '*.LC24' defined in '../../../../../../../modules/lib/open-amp/open-amp/lib/rpmsg/rpmsg_virtio.c' (0x8202c60) of size 3
  '*.LC24' is ascii string 'NS'
SUMMARY: AddressSanitizer: global-buffer-overflow ../../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:389 in __interceptor_strlen

@laxiLang could you tell me how to reproduce the test

It can resolved with the changes in zephyrproject-rtos/open-amp#23

Copy/past from comment https://github.com/zephyrproject-rtos/open-amp/pull/23/commits/cff00b649ee90c3d5cbf5e693c3b3754c736863f#r1792242968:

The function has to return the total length of the string it tried to create so the size of the source (https://man.freebsd.org/cgi/man.cgi?strlcpy(3)) The freebsd add in code while (*src++) resulting in same overflow issue than strlen, but perhaps not... If you detect a potential issue, please, comment OpenAMP/open-amp#620 with your observation that I fix it in the openamp repo

[laxiLang]

@arnopo : I enabled the GCC address sanitizer check. It can be easily reproduced if run a test function as below:

int main(int argc, char ** argv)
{
    union
    {
      char src[2];
      int  i;
    } u;
    u.i = 0xFFFFFFFF;
    char dst[100];
    strlcpy(dst, &u.src[0], sizeof(dst));
}

got such below error: #######################################################################################

==904719==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffffffda54 at pc 0x555555555299 bp 0x7fffffffd9d0 sp 0x7fffffffd9c0

READ of size 1 at 0x7fffffffda54 thread T0

    #0 0x555555555298 in strlcpy /home/jos1/repos/jos1-public/samples/strlcpy.c:11

    #1 0x555555555493 in main /home/jos1/repos/jos1-public/samples/strlcpy.c:37

    #2 0x7ffff7029d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

    #3 0x7ffff7029e3f in __libc_start_main_impl ../csu/libc-start.c:392

    #4 0x555555555164 in _start (/home/jos1/repos/jos1-public/samples/strlcpy_sanitized+0x1164)

Address 0x7fffffffda54 is located in stack of thread T0 at offset 36 in frame

    #0 0x555555555397 in main /home/jos1/repos/jos1-public/samples/strlcpy.c:26

  This frame has 2 object(s):

    [32, 36) 'u' (line 31) <== Memory access at offset 36 overflows this variable

    [48, 148) 'dst' (line 35)

HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork

      (longjmp and C++ exceptions *are* supported)

SUMMARY: AddressSanitizer: stack-buffer-overflow /home/jos1/repos/jos1-public/samples/strlcpy.c:11 in strlcpy

Shadow bytes around the buggy address:

  0x10007fff7af0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

  0x10007fff7b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

  0x10007fff7b10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

  0x10007fff7b20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

  0x10007fff7b30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

=>0x10007fff7b40: 00 00 00 00 00 00 f1 f1 f1 f1[04]f2 00 00 00 00

  0x10007fff7b50: 00 00 00 00 00 00 00 00 04 f3 f3 f3 f3 f3 00 00

  0x10007fff7b60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

  0x10007fff7b70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

  0x10007fff7b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

  0x10007fff7b90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Shadow byte legend (one shadow byte represents 8 application bytes):

  Addressable:           00

  Partially addressable: 01 02 03 04 05 06 07 

  Heap left redzone:       fa

#######################################################################################

To make address sanitizer check passed, the below change with still return total length of the string works.

 strlcpy(char *dst, const char *src, size_t size)
 {
        size_t nleft = size;
+     const char *s= src;
         /* Copy as many bytes as will fit. */
        if (nleft != 0) {
                while (--nleft != 0) {
-                       *dst = *src++;
+                       *dst = *s++;

[arnopo]

The strlcpyshould return the length of the source, but in the end, it seems not possible to know the source size. more than that we could read at some address that is out of the src string memory (this seems the case in @laxiLang example).

Based on this, it looks to me that strlcpy is not 100% safe. A safer approach would be to provide the size of the source string in addition to the destination size, and then return the size of the destination string.

metal_weak size_t safe_strcpy(char *dst, size_t d_size, const char *src, size_t s_size)
{
    size_t size = metal_min(s_size, d_size);
    size_t nleft = size + 1;
    char *d = dst;

    /* Copy as many bytes as will fit. */
    if (nleft != 0) {
        while (--nleft != 0) {
            *dst = *src++;
            if (*dst++ == '\0')
                break;
        }
    }

    /*Fill last characters with '\0' */
    if (size < d_size)
        memset(dst, '\0',  d_size - size + nleft);
    else
        d[d_size - 1] = '\0';

    return size - nleft;
}

[arnopo]

I have updated the PR in this way