Message-ID: <CAG48ez17ZtF4YESkGeegJYMTuQMsRcU3njVkNqbSbgv75jpc5g@mail.gmail.com>
Date: Sat, 22 Mar 2025 04:38:21 +0100
From: Jann Horn <jannh@...gle.com>
To: Kees Cook <kees@...nel.org>
Cc: Vlastimil Babka <vbabka@...e.cz>, Miguel Ojeda <ojeda@...nel.org>, 
	Nathan Chancellor <nathan@...nel.org>, Marco Elver <elver@...gle.com>, 
	Nick Desaulniers <ndesaulniers@...gle.com>, Przemek Kitszel <przemyslaw.kitszel@...el.com>, 
	Christoph Lameter <cl@...ux.com>, Pekka Enberg <penberg@...nel.org>, David Rientjes <rientjes@...gle.com>, 
	Joonsoo Kim <iamjoonsoo.kim@....com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Roman Gushchin <roman.gushchin@...ux.dev>, Hyeonggon Yoo <42.hyeyoo@...il.com>, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH 3/5] compiler_types: Introduce __is_lvalue()

On Fri, Mar 21, 2025 at 9:41 PM Kees Cook <kees@...nel.org> wrote:
> If __builtin_is_lvalue() is available, use it with __is_lvalue(). There
> is patch to Clang to provide this builtin now[1].
>
> Link: https://github.com/kees/llvm-project/commits/builtin_is_lvalue/ [1]
> Signed-off-by: Kees Cook <kees@...nel.org>

Before you land that LLVM patch, it might be a good idea to figure out
how this interacts with the fun C quirk where you can have temporary
rvalues which can contain array members to which you can technically
create lvalues but must not write. As far as I understand, calling
"kfree(getS().ptrs[0])" as in the following example would cause UB
with your patch:

```
$ cat kees-kfree-test.c
#include <stdlib.h>

#define __is_lvalue(expr)      __builtin_is_lvalue(expr)

void __kfree(void *ptr);
void BEFORE_SET_TO_NULL();
void AFTER_SET_TO_NULL();
static inline void kfree_and_null(void **ptr)
{
       __kfree(*ptr);
       BEFORE_SET_TO_NULL();
       *ptr = NULL;
       AFTER_SET_TO_NULL();
}
#define __force_lvalue_expr(x) \
       __builtin_choose_expr(__is_lvalue(x), x, (void *){ NULL })
#define __free_and_null(__how, x)      \
({                                     \
       typeof(x) *__ptr = &(x);        \
       __how ## _and_null((void **)__ptr);     \
})
#define __free_and_maybe_null(__how, x)        \
       __builtin_choose_expr(__is_lvalue(x), \
               __free_and_null(__how, __force_lvalue_expr(x)), \
               __kfree(x))
#define kfree(x)          __free_and_maybe_null(kfree, x)

struct S {
    void *ptrs[1];
};
struct S getS(void);

int is_lvalue_test(void) {
  return __is_lvalue(getS().ptrs[0]);
}
void testfn2(void) {
  kfree(getS().ptrs[0]);
}
$ [...]/bin/clang-14 -c -o kees-kfree-test.o kees-kfree-test.c -O3 -Wall
$ objdump -d -Mintel -r kees-kfree-test.o

kees-kfree-test.o:     file format elf64-x86-64


Disassembly of section .text:

0000000000000000 <is_lvalue_test>:
   0:   b8 01 00 00 00          mov    eax,0x1
   5:   c3                      ret
   6:   66 2e 0f 1f 84 00 00    cs nop WORD PTR [rax+rax*1+0x0]
   d:   00 00 00

0000000000000010 <testfn2>:
  10:   50                      push   rax
  11:   e8 00 00 00 00          call   16 <testfn2+0x6>
                        12: R_X86_64_PLT32      getS-0x4
  16:   48 89 c7                mov    rdi,rax
  19:   e8 00 00 00 00          call   1e <testfn2+0xe>
                        1a: R_X86_64_PLT32      __kfree-0x4
  1e:   31 c0                   xor    eax,eax
  20:   e8 00 00 00 00          call   25 <testfn2+0x15>
                        21: R_X86_64_PLT32      BEFORE_SET_TO_NULL-0x4
  25:   31 c0                   xor    eax,eax
  27:   59                      pop    rcx
  28:   e9 00 00 00 00          jmp    2d <testfn2+0x1d>
                        29: R_X86_64_PLT32      AFTER_SET_TO_NULL-0x4
jannh@...n:~/test/kees-kfree$
```

As far as I understand, this causes UB in C99 ("If an attempt is made
to modify the result of a function call or to access it after the next
sequence point, the behavior is undefined.") and in C11 ("A non-lvalue
expression with structure or union type, where the structure or union
contains a member with array type (including, recursively, members of
all contained structures and unions) refers to an object with
automatic storage duration and temporary lifetime. 36) Its lifetime
begins when the expression is evaluated and its initial value is the
value of the expression. Its lifetime ends when the evaluation of the
containing full expression or full declarator ends. Any attempt to
modify an object with temporary lifetime results in undefined
behavior.").

Basically, something like getS().ptrs[0] gives you something that is
technically an lvalue but must not actually be written to, and
->isModifiableLvalue() does not catch that.

Powered by blists - more mailing lists