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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 39AA3EB64DD for ; Thu, 13 Jul 2023 05:22:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233858AbjGMFWk (ORCPT ); Thu, 13 Jul 2023 01:22:40 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53966 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233606AbjGMFWj (ORCPT ); Thu, 13 Jul 2023 01:22:39 -0400 Received: from mail-pf1-x42b.google.com (mail-pf1-x42b.google.com [IPv6:2607:f8b0:4864:20::42b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 21910B0 for ; Wed, 12 Jul 2023 22:22:38 -0700 (PDT) Received: by mail-pf1-x42b.google.com with SMTP id d2e1a72fcca58-67ef5af0ce8so351401b3a.2 for ; Wed, 12 Jul 2023 22:22:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1689225757; x=1691817757; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=xXkqor9Pj1M925UJ+WZIwXJFUm4FisVGEdFy8WXsojI=; b=j8k6Q14byW5JjV/sfjFd4uGLMjuj8iRdgH6g3n6siLv7deQY2HQwHXhjx/QjKK7yfB DorLX40CwV+wcqDrMqndTl2/0ptE00PIfVl/pslyzO6lcjmOANqhhBMO5hl+yVZAo62L Hqh3mieIp+KzYzu8DAykAl1wyj+466CI0BS64= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689225757; x=1691817757; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=xXkqor9Pj1M925UJ+WZIwXJFUm4FisVGEdFy8WXsojI=; b=CiDCCF5gpNWdpYOciyhRQg/98zT6YRaCIRMsNQUcaH4OMKQZjbX0cBuJ5cWjHICMyz Nt8PpO89MVaS742hyHUq0sZX8tI5tYbjYGIqBLuaZulknRh0ysQbiVkD8GzQtzrLMYgk jCdTz1LvVEuuWR2JxFwwXfYi5fVeacXi/6NCsV+iA9/xMUVZc8ft/TQu/YArFUCpBawc He2XEG9XEufDxtMGLZEIcqza4aZmWN9RG5Qt90NmOTlN53l+KeUtbc5CILsK0txALQdU ExdzELGsQ/qIAcHO7pfERhE5UulTOkQp6whgb0EuVJ2CjvUUpAkzXZ+XmAMYsreLAONM tpig== X-Gm-Message-State: ABy/qLblz57m3QVdRBP30s5/pX9k5zc8mgrJjF8IEJcZ6s7aNo5QI5xn yYObguWBuoEvUfMzJIQnI7WnBw== X-Google-Smtp-Source: APBJJlHplYhOqSZW6ahZfnyEya/TDhnDEgnXh2VWL3LotXdHTEHPrKtPps+qfXQ93yZwO0iIoJkFTA== X-Received: by 2002:a05:6a20:32aa:b0:12e:32e2:2645 with SMTP id g42-20020a056a2032aa00b0012e32e22645mr432420pzd.15.1689225757577; Wed, 12 Jul 2023 22:22:37 -0700 (PDT) Received: from www.outflux.net (198-0-35-241-static.hfc.comcastbusiness.net. [198.0.35.241]) by smtp.gmail.com with ESMTPSA id k22-20020aa792d6000000b00682868714fdsm4626428pfa.95.2023.07.12.22.22.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 12 Jul 2023 22:22:36 -0700 (PDT) Date: Wed, 12 Jul 2023 22:22:35 -0700 From: Kees Cook To: Steven Rostedt Cc: LKML , Linux Trace Kernel , Masami Hiramatsu , Mark Rutland , Sven Schnelle , linux-hardening@vger.kernel.org Subject: Re: [PATCH] tracing: Stop FORTIFY_SOURCE complaining about stack trace caller Message-ID: <202307122118.F4DD6200@keescook> References: <20230712105235.5fc441aa@gandalf.local.home> <202307121618.17C50DA9A@keescook> <20230712204358.756d0018@gandalf.local.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230712204358.756d0018@gandalf.local.home> Precedence: bulk List-ID: X-Mailing-List: linux-trace-kernel@vger.kernel.org On Wed, Jul 12, 2023 at 08:43:58PM -0400, Steven Rostedt wrote: > On Wed, 12 Jul 2023 16:36:30 -0700 > Kees Cook wrote: > > [...] > > +union stack_entry_dynamic { > > + struct stack_entry entry; > > + struct { > > + int size; > > + unsigned long caller[] __counted_by(size); > > + }; > > +}; > > This actually makes it more likely to cause a bug in the future (and the > present!). Now we need to know that if stack_entry were to ever change, we > have to change this too. And it can change (although highly unlikely). Yeah, I should have included a bit more, like: BUILD_BUG_ON(offsetof(struct stack_entry, caller) == offsetof(struct stack_entry_dynamic, caller)) But anyway, I think we can still do better. :) > [...] > Now, this event is unique, as it's the only event that has a dynamic array > that is not done by the way other dynamic arrays are done. Which is to > insert a field that has an offset and length to it. That is, other events > would look like this: > > struct stack_entry { > int size; > short offset; length; > [ more fields ] > int dynamic[]; > } Yeah, it won't be a "true" trace dynamic array. stack_entry is the sockaddr of trace. :) (i.e. we want to change the size of the trailing array without actually changing the struct, but this way leads to madness.) > Where offset would be the ((void *)(struct stack_entry *)data) + offset. As > all the dynamic size portions of an event are at the end of the event, with > these offset/length tuples to tell user space and the kernel where to look > in the event binary data for those fields. > > But to keep backward compatibility, as this event had code specific for > parsing it in libtraceevent that doesn't expect that offset/length tuple, > and instead just looked at the "caller[8]" portion to see that it had 8 > fields (this is static for all events). New code uses the size to know, and > ignores the [8]. The event meta data gives the actual size of the stored > data so the parser knows not to go beyond that. Right -- so, it would have been much nicer to use a new struct when caller[8] wasn't enough. :) But that ship has sailed. Now we're in a state where some tools expect caller[8] and some expect "caller[] __counted_by(size)". In other places where we've had a "minimum sized flexible array" requirements, we would've used a union, effectively like: int size; union { some_type legacy_padding[8]; some_type caller[] __counted_by(size); }; Old code ends up with the same sized struct, and new code gets a proper flexible array with the original struct member name. (And with __counted_by, we'll have the capacity to do run-time bounds checking on the flexible array.) > > Note, this event existed before normal trace events that had the dynamic > arrays, which is why it didn't do the same. > > The only thing this code is doing is filling in the ring buffer. The entry > structure is just a helper to know where to place the data in the ring > buffer. > > So my question to you is, what's the purpose of hardening? To prevent > future bugs, right? By making a shadow struct, we are now burdened to > remember to modify the shadow stack if we ever modify this current > structure. Right, I'd rather avoid a shadow struct -- it's especially weird here given the special way trace creates its structures. :P The hardening efforts are mainly holistic protections, in the sense that much of the kernel isn't doing this kind of careful length management, etc. But that means we can't abuse fixed-sized arrays anymore -- the compiler needs to believe what it's told about sizes. ;) > [...] > This is why I much rather prefer the simple: > > ptr += offsetof(typeof(*entry), caller); > memcpy(ptr, fstack->calls, size); > > Which doesn't need to care about synchronizing with the macro magic of the > structure, which may change in the future, and this will lead to one more > location that would need to be updated, but likely forgotten. I am just thinking this will kick the can down the road. The compiler may get smart enough to see through this or who know what next. > C is fun, let's go shopping! Yup. I'm just trying to take away all the foot-guns. :) Let me try another way to solve this that is less of a bypass. For now, sure, let's take the work-around, but I think it'll need to be fixed up soon. -Kees -- Kees Cook