| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <202503212348.C21AACA6@keescook>
Date: Sat, 22 Mar 2025 00:03:09 -0700
From: Kees Cook <kees@...nel.org>
To: Jann Horn <jannh@...gle.com>
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,
Andrew Pinski <pinskia@...il.com>
Subject: Re: [PATCH 3/5] compiler_types: Introduce __is_lvalue()
On Sat, Mar 22, 2025 at 04:38:21AM +0100, Jann Horn wrote:
> 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:
Yay UB! I can confirm that currently isModifiableLvalue() does not catch
this.
>
> ```
> $ 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
I don't see UB manifested here, though? It looks more like a dead store
was eliminated (i.e. it was an automatic variable that wasn't going to
be referenced outside of the expression statement).
If I add a global and assign the global from *ptr, it all seems to work
fine:
--- kees-kfree-test.c.orig 2025-03-22 00:00:53.550633347 -0700
+++ kees-kfree-test.c 2025-03-21 23:58:57.124268268 -0700
@@ -2,6 +2,8 @@
#define __is_lvalue(expr) __builtin_is_modifiable_lvalue(expr)
+void *g;
+
void __kfree(void *ptr);
void BEFORE_SET_TO_NULL();
void AFTER_SET_TO_NULL();
@@ -11,6 +13,7 @@
BEFORE_SET_TO_NULL();
*ptr = NULL;
AFTER_SET_TO_NULL();
+ g = *ptr;
}
#define __force_lvalue_expr(x) \
__builtin_choose_expr(__is_lvalue(x), x, (void *){ NULL })
...
27: e8 00 00 00 00 call 2c <testfn2+0x1c>
28: R_X86_64_PLT32 AFTER_SET_TO_NULL-0x4
2c: 48 c7 05 00 00 00 00 mov QWORD PTR [rip+0x0],0x0 # 37 <testfn2+0x27>
33: 00 00 00 00
> 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.
But I agree, any mention of UB does give me pause! :)
--
Kees Cook
Powered by blists - more mailing lists