[Bug]: tsip_usable(): the NULL check cannot run, and returns the wrong value if it did

[baigeb]

Contact Details

18102059357@163.com

Version

5.9.2

Description

[Bug]: tsip_usable(): the NULL check cannot run, and returns the wrong value if it did

The line above is the issue title. Everything below it is the Description
field; sections 5 and 6 are the Reproduction steps and Relevant log output
boxes.

1. Summary

Two issues in tsip_usable(): a NULL check that cannot run (the compiler
removes it), and a return value that contradicts the function's own
comment.

2. Problems

Problem 1 — the NULL check is too late to matter (renesas_tsip_util.c:2563–2573)

byte cipher0 = ssl->options.cipherSuite0;  /* line 2563: ssl already used */
byte cipher  = ssl->options.cipherSuite;
byte side    = ssl->options.side;
/* ... */

/* sanity check */
if (ssl == NULL) {                        /* line 2573: check is too late */
    WOLFSSL_MSG("ssl is NULL");
    ret = BAD_FUNC_ARG;
}

The function reads three fields through ssl before it checks ssl for
NULL, so the check cannot protect those reads. Reading a field through a NULL
pointer is also not allowed in C, which lets the compiler assume ssl is
non-NULL by the time the check runs and delete it, together with its message.

Measured on a small program with the same read-then-check ordering, compiled
at -O2 with and without the guard: the .text of the two objects is
byte-identical, so the guard produces no code at all.

original          .text 28 bytes  sha256 4bd8b10aed99a8b4
without the check .text 28 bytes  sha256 4bd8b10aed99a8b4

<tsip_usable>:
   0:	endbr64
   4:	movzbl (%rdi),%eax           <- ssl->options; no NULL test anywhere
   ...
  1b:	ret

Problem 2 — the value the check returns contradicts the comment (renesas_tsip_util.c:2560–2575)

/* return  :1 when tsip can be used , 0 not be used.           */   /* line 2560 */

The NULL branch sets ret = BAD_FUNC_ARG at line 2575. That is a negative
error code, but the callers use the result as true/false
(if (tsip_usable(ssl, 0))), so a value that means "cannot use TSIP" would be
read as "can use TSIP". Moving the check earlier is therefore not enough on
its own; the value it returns has to change too.

3. Suggested fix

Check ssl before reading its fields, and return 0 from the NULL branch.
Replace lines 2563–2576 with:

    byte cipher0;
    byte cipher;
    byte side;
    int ret = WOLFSSL_SUCCESS;
    const Ciphers *enc;
    const Ciphers *dec;

    WOLFSSL_ENTER("tsip_usable");

    /* sanity check */
    if (ssl == NULL) {
        WOLFSSL_MSG("ssl is NULL");
        WOLFSSL_LEAVE("tsip_usable", 0);
        return 0;
    }

    cipher0 = ssl->options.cipherSuite0;
    cipher  = ssl->options.cipherSuite;
    side    = ssl->options.side;

The rest of the function is unchanged. With the reads below the check the
guard is no longer dead: the same comparison now says differs (43 bytes
against 28 bytes).

This is not a security problem today: renesas_common.c:1071 and
renesas_tsip_util.c:2056, :2096 all pass a live session or test ssl
first, and a NULL would crash on the first line whether or not the check
exists. The value of the change is that the function does what its own
comment says.

4. Configuration

The file is only compiled when WOLFSSL_RENESAS_TSIP_TLS is defined through a
user_settings.h, as in the shipped Renesas projects
(IDE/Renesas/e2studio/RX65N/RSK/wolfssl_demo/user_settings.h), so a default
./configure build does not cover it. This comes from reading the source, not
from a failing build.

Happy to send a patch if you want one.

verify.sh

Reproduction steps

1. Write the same read-then-check ordering in a small program, with the guard
   marked between /* guard-begin / and / guard-end */.
2. gcc -O2 -c repro.c -o a.o
3. Remove the marked block and compile again: gcc -O2 -c repro.c -o b.o
4. objcopy -O binary --only-section=.text a.o a.text (the same for b.o)
5. cmp a.text b.text -> identical, i.e. the guard produced no code.
   With the reads moved below the check, that comparison says "differs"
   instead (43 bytes vs 28 bytes).

Relevant log output

$ bash verify.sh reproducer.c
original      28 bytes  4bd8b10aed99a8b4
patched       28 bytes  4bd8b10aed99a8b4
identical: the guard is dead, it produces no code

0000000000000000 <tsip_usable>:
   0:	endbr64
   4:	movzbl (%rdi),%eax           <- ssl->options; no NULL test anywhere
   7:	cmpq   $0x1,0x8(%rdi)
   c:	movzbl 0x1(%rdi),%edx
  10:	sbb    $0xffffffff,%eax
  13:	add    %edx,%eax
  15:	movzbl 0x2(%rdi),%edx
  19:	add    %edx,%eax
  1b:	ret