From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id A5E3EC35FFC for ; Sat, 22 Mar 2025 07:03:17 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 333FF280002; Sat, 22 Mar 2025 03:03:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2E453280001; Sat, 22 Mar 2025 03:03:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1AC89280002; Sat, 22 Mar 2025 03:03:15 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id EFB81280001 for ; Sat, 22 Mar 2025 03:03:14 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 157A3140FD3 for ; Sat, 22 Mar 2025 07:03:16 +0000 (UTC) X-FDA: 83248295592.29.0760B56 Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf12.hostedemail.com (Postfix) with ESMTP id E6B4640008 for ; Sat, 22 Mar 2025 07:03:13 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=uh8bkayS; spf=pass (imf12.hostedemail.com: domain of kees@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=kees@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1742626994; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=M01soAl5Y/AYC0Otz84RkpGiAKiN6NqE0yRwt7kwLvo=; b=nSsAVkzyqpJKMMrsWQnujRyIgtpzvIc20YSHX9Fa9lJzd1tKiCsEmTKLNRAMV7/OsDZFmx fa/Cb9HI6nN8ZYWnm7kuPNvbaSHuf+AAst153s3bLSumEVCEhzs8tOAXLTmIFME0BJz5/J ASDSnMnrV9LtO4cZVa97qyEmjGNiZ7A= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=uh8bkayS; spf=pass (imf12.hostedemail.com: domain of kees@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=kees@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1742626994; a=rsa-sha256; cv=none; b=ukiliisu0K/pWSve8fQ4dX0qZYywG7g6wPQq02+xan57VLKH8nG2W26mqX9Nth3wHIakF5 VLGYGmc+8UAXI1yiYoaYavPUqxy8wx1nCqB0HE2NDH3aSYjJbBBzOFFXG1uNdP0MSkhn9O XdFrBST710vVzjWWVpgrX7PNGlichqY= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 062EEA49743; Sat, 22 Mar 2025 06:57:44 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id E4C8CC4CEDD; Sat, 22 Mar 2025 07:03:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1742626993; bh=HtzlmGFKNc96G3c31xfHA2/WSc+840Arh4hTDS/Blu4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=uh8bkaySVZxDeFbmWF40056K7l8MVIb+9kF4CPx6jnwyybECJMrvd1REqb6+ZGb/7 U1M5Zwze0ONfLVHeZABlmUHipLqjLu0fd0Ibaf/a8/dI/4dcMUjq0xIm74Ox02xp5t 7WMD1NaJ6nuv24ylk6fz4ZSXJ+HF8jQBfEe2F2ahX3sBX/vgAT8SZ/0CGzmQTA/et7 epxym2eHwY69WirMTSZHNyvsMbhYU5XJ/Dvy4qaTqU4nonav25pjjM0Vt4MfXM3sd0 jTzv9+YZqKA8tcvByuN4WGOc+/V1HIX6UD47YGXBnnEC2Hat5xdrTMF9NRqIQJ1PN+ FVCchGpV2CxWQ== Date: Sat, 22 Mar 2025 00:03:09 -0700 From: Kees Cook To: Jann Horn Cc: Vlastimil Babka , Miguel Ojeda , Nathan Chancellor , Marco Elver , Nick Desaulniers , Przemek Kitszel , Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Roman Gushchin , Hyeonggon Yoo <42.hyeyoo@gmail.com>, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-hardening@vger.kernel.org, Andrew Pinski Subject: Re: [PATCH 3/5] compiler_types: Introduce __is_lvalue() Message-ID: <202503212348.C21AACA6@keescook> References: <20250321202620.work.175-kees@kernel.org> <20250321204105.1898507-3-kees@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspamd-Queue-Id: E6B4640008 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: f3hbci6unj4udr3ahw5imm81ozfxmk9p X-HE-Tag: 1742626993-360359 X-HE-Meta: U2FsdGVkX19dApbbqmV1sRbuHbQ9ptOKhP5VVBrShDxdCISI9yPC91ZQnT6UVitC6N7yu0Xf501iFANUqJHh8OaM6J540IB8PgSo+56d2vD9fOJ74M21RyHddSbpDD1DKVaLbsDDkDRdzF77ttWd1Kmw3sGkIGfFcX7a6UY/Z886aXXdB34KK0QSLzP8M4xxAfM8z3xlx3f3rmOxmUciT5Jf+kzfNXT90IMAuEMo4sbYqZtyaflLdnnHMbcSAEcRZgUDM+3e87LVl9T2Eb6+/Gkcyv2ntUDj3E5WsKdYqX5yhnUz8q1pfFn7VVphtWDuxDss1/8aZGNZZTp/e7GTarJ4a7E4t3UHAq1ycqP91Fz9PR52hpH6lEivphIWtmHxLEXsXfgx0H8wC3xnTIf+xJJL5/0DQNP0UAbdDcmL/KgCsge6pjlXNT5+mgpnzvwkwdW26BUxrxaENf9zyuCq/obMAebIZyp14sMEVHIHxeOVI9h8V+nFCACtmObmSh94Uitlk1xGmuuNObzrpR002q7alhg+OCoVQH4o//yHnB6JB3H1PaYpcjzeNhc3bdheKkyDruO9e86dhn5xTkxDqVtnm3G0fBORx9DoA/oWIycAxL/TckKT+yHedPllwgG73yQO1y37wNenMwcRbj5s8O4mPqm2XaGtHbaEWTOY612Kl1fep2J8GWfIihpnOLe3Pr4VGu2tYLZatmBZuURW46Qhorn3JdpDUQocfhKQKqiPY3Foj34264oPosnp0TEv4xSkMg5rVysIwyFTZEkP5qZ4NaS9dImiGB+2UVN6CMcoyjyUJa1cA1TILOY5BbMXHRD+nBmoyVPtkfCK7MEzNQGaXfWXCN66dbx1zQNk3P6wBLrSqolF0VN0k4JPb0cmQXwhRRpVjsAWR5wVZBYWXYm/86sBNPkAwiT9z1gQJPIhioGyk7jeEQhmc+7+G6Aj7jEFSgReaUVNO1LeWbN jQlgkExG Aa2aMy3pYN6XkVwIt6AZ54cs6k3Uxc1dTlt72YC5aktlI3VzpbhuwRDwTvS61gjXtamTwa9UcKqbbj9UzZKLrua+WiIHbg2QGvV/4xrfrdrXXU3IuoMUrBZRIS/jf3C1Uu1HuL/erI4Fo1Fo/0XTEZz30/1WHIrIPzu5pYJfpTM70ZYj5oHg9gPw/NTLh7ma9wfE4VqxS15SZDUzW/xQptiMH60erQpL1LMQHTus8WnsaJpYi3EjJqTiX4+KW0BoRANGyYApZm7ofETHE2/GMlC4PGzuYMRVuFMYQ4V6OUiqRpYQIDtBXkj2w0QxpcKUkJZHJuijZi5bYONzTxVW1LtB0e1TGBoftD9zsNErLikACxTuH1De7WAVrPsI9W+9lzD4Vfmlh6k6r5qcKFIOyjNdhxOzDSx/zRxWbqpIt+IyCKs/l8COISOm0IemVgxnzW2vZ X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Sat, Mar 22, 2025 at 04:38:21AM +0100, Jann Horn wrote: > On Fri, Mar 21, 2025 at 9:41 PM Kees Cook 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 > > 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 > > #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 : > 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 : > 10: 50 push rax > 11: e8 00 00 00 00 call 16 > 12: R_X86_64_PLT32 getS-0x4 > 16: 48 89 c7 mov rdi,rax > 19: e8 00 00 00 00 call 1e > 1a: R_X86_64_PLT32 __kfree-0x4 > 1e: 31 c0 xor eax,eax > 20: e8 00 00 00 00 call 25 > 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 > 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 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 33: 00 00 00 00 > jannh@horn:~/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