From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 1967 invoked from network); 14 Sep 2023 12:18:45 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 14 Sep 2023 12:18:45 -0000 Received: (qmail 18096 invoked by uid 550); 14 Sep 2023 12:18:42 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 18057 invoked from network); 14 Sep 2023 12:18:41 -0000 Date: Thu, 14 Sep 2023 08:18:35 -0400 From: Rich Felker To: Joakim Sindholt Cc: musl@lists.openwall.com, James R T Message-ID: <20230914121834.GE4163@brightrain.aerifal.cx> References: <20230908174939.80579-1-jamestiotio@gmail.com> <20230909004835.GD4163@brightrain.aerifal.cx> <20230914112308.bc1833d3f827ada2d1b7d85c@zhasha.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230914112308.bc1833d3f827ada2d1b7d85c@zhasha.com> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] Add a safe dequeue integrity check for mallocng On Thu, Sep 14, 2023 at 11:23:08AM +0200, Joakim Sindholt wrote: > On Thu, 14 Sep 2023 13:13:23 +0800, James R T wrote: > > On Sat, Sep 9, 2023 at 8:48 AM Rich Felker wrote: > > > > > > This could and should be written with the assert macro, like all the > > > other safety assertions in mallocng, not pulling in stdio and abort. > > > > Understood. I was not able to find an assert with `predict_false` for > > the condition. Should I add one assert function with `predict_false` > > in `include/assert.h` or `src/exit/assert.c` or simply use the regular > > assert? > > It's a little confusing but assert() in mallocng is not real assert(): > http://git.musl-libc.org/cgit/musl/tree/src/malloc/mallocng/glue.h#n33 > The issue is that if memory is under control of an attacker then doing > anything at all, especially running the stdio machinery, is unsafe. To > that end musl uses a_crash() here which expands to a minimal set of > instructions to crash the process: > http://git.musl-libc.org/cgit/musl/tree/arch/x86_64/atomic_arch.h#n106 Yes. mallocng is written such that you could use the normal assert with it, but presently it's just expanding to a_crash(). At some point this might be revamped to crash with a message string in a particular register or argument slot or something so that you get a bit more meaningful information if looking at it in a debugger. And indeed, the reason not to do any message printing, etc. is that you're running in a known-compromised process state where any further complex execution is unsafe (e.g. if the out-of-band malloc metadata was clobbered, the function pointers in stderr might also have been clobbered, since the latter are *easier to reach* than the OOB metadata). > Furthermore, musl doesn't use any of thosed tagged branch tricks and I > personally doubt it would make any difference. Yes, the only reason libm.h has them is because nsz is using the code in other environments that want them, and it made sense to avoid gratuitous differences. We don't generally use those in musl. If the compiler isn't generating good code and puts the failure path as a hot path, we probably should explore whether the compiler is missing that it's a does-not-return thing (which should always be treated as cold). But indeed I doubt it makes a difference. Rich