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 772F7C433F5 for ; Tue, 10 May 2022 22:18:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235375AbiEJWS1 (ORCPT ); Tue, 10 May 2022 18:18:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37704 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235434AbiEJWS0 (ORCPT ); Tue, 10 May 2022 18:18:26 -0400 Received: from mail-pf1-x42f.google.com (mail-pf1-x42f.google.com [IPv6:2607:f8b0:4864:20::42f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 824B76C54C for ; Tue, 10 May 2022 15:18:24 -0700 (PDT) Received: by mail-pf1-x42f.google.com with SMTP id bo5so354645pfb.4 for ; Tue, 10 May 2022 15:18:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=InNCa+xjWOlYGnWYWDC3xJ/eS8N5Jwfc81JekwauSRA=; b=XgtYlZ/dCAkrzwvZEDyVsySj5TSiVF/dDKregrx22WRD39pVwUZcxemUeP7nTgQjba ZCpQo5QtAyL2qT30j6wV9nr0O6tFtX/kYKOb8nswyOkrIjoIPG+7SMbO6lnuGW3up+lq HeUh1iG/VMrfEarCdU3yR1VhJZuE1Z9Y6U6ww= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=InNCa+xjWOlYGnWYWDC3xJ/eS8N5Jwfc81JekwauSRA=; b=I//rjWzMU0HnGcBX8B9ynmqzHTp9CTmGzilf8P8opbjnGQTDAGIL5mkF46xX3XYjr0 lSnPr3zS6JktcmNiwwXn3Ffb/HM6tYz1130rmc83nCO0kW3XyL6tR21wOt7Ksgy4Akq9 89k2ZcemX1BZXXVvK5T9sGmUzft3lajB8DqyudD+S0M8xn7tCNAS9Lam0OpL5q9n2/GZ 0On0kbzskott7h5RzONh+yYFXHlb3JrWxdx0xFS7AnsHRmyKOlaRb0IixfcbKCm/6CPD OP6m5OAytIY8mMRETmTBL7Lta6Z96iOUdHqvy2PG6CJfMWXKZO2MuAtaKZcURUWbTMsD vHVA== X-Gm-Message-State: AOAM531F1ZPE8hTXVwuPPSU+12bI6FLGtI0oGVDp8FTlvWeot0WNO0qY +qWnVJp//uYxyNslzeFJRDszNA== X-Google-Smtp-Source: ABdhPJykDdBjmf6E/+QD1MPnRy3T6XJmUj5VkOlEr3aIPGjL/svdAg5YZdWeuCvHLVkvcoHgfPX0CQ== X-Received: by 2002:a63:181c:0:b0:3da:fe5d:90f3 with SMTP id y28-20020a63181c000000b003dafe5d90f3mr2938243pgl.350.1652221104014; Tue, 10 May 2022 15:18:24 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id o6-20020a1709026b0600b0015e8d4eb263sm120406plk.173.2022.05.10.15.18.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 10 May 2022 15:18:23 -0700 (PDT) Date: Tue, 10 May 2022 15:18:22 -0700 From: Kees Cook To: Eric Biggers Cc: David Howells , Christoph Hellwig , Jarkko Sakkinen , James Morris , "Serge E. Hallyn" , linux-hardening@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] big_keys: Use struct for internal payload Message-ID: <202205101509.B5E3096@keescook> References: <20220508175732.2693426-1-keescook@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: keyrings@vger.kernel.org On Mon, May 09, 2022 at 04:13:48PM -0700, Eric Biggers wrote: > On Sun, May 08, 2022 at 10:57:31AM -0700, Kees Cook wrote: > > The randstruct GCC plugin gets upset when it sees struct path (which is > > randomized) being assigned from a "void *" (which it cannot type-check). > > > > There's no need for these casts, as the entire internal payload use is > > following a normal struct layout. Convert the enum-based void * offset > > dereferencing to the new big_key_payload struct. No meaningful machine > > code changes result after this change, and source readability is improved. > > > > Drop the randstruct exception now that there is no "confusing" cross-type > > assignment. > > > > Cc: David Howells > > Cc: Jarkko Sakkinen > > Cc: James Morris > > Cc: "Serge E. Hallyn" > > Cc: linux-hardening@vger.kernel.org > > Cc: keyrings@vger.kernel.org > > Cc: linux-security-module@vger.kernel.org > > Signed-off-by: Kees Cook > > --- > > scripts/gcc-plugins/randomize_layout_plugin.c | 2 - > > security/keys/big_key.c | 64 ++++++++++--------- > > 2 files changed, 34 insertions(+), 32 deletions(-) > > This looks fine to me, although the way that an array of void pointers is cast > to/from another struct is still weird. I'd prefer if the payload was just > changed into a separate allocation. Yeah, though I realized after sending this patch that I'd done it before[1] back with the rest of the randstruct GCC plugin enabling, and it seems David was against the separate allocation, which, given the space available, isn't unreasonable right up until struct path doesn't fit anymore, but that's why I've added the BUILD_BUG_ON() to check sizes. :) And this version ended up quite close to what hwh suggested[2] in 2017. > A couple nits below if you stay with your proposed solution: > > > void big_key_free_preparse(struct key_preparsed_payload *prep) > > { > > + struct big_key_payload *payload = to_big_key_payload(prep->payload); > > + > > if (prep->datalen > BIG_KEY_FILE_THRESHOLD) { > > - struct path *path = (struct path *)&prep->payload.data[big_key_path]; > > + struct path *path = &payload->path; > > > > path_put(path); > > } > > This could just do: > > if (prep->datalen > BIG_KEY_FILE_THRESHOLD) > path_put(&payload->path); Sure, I can avoid the extra variable. > > > void big_key_destroy(struct key *key) > > { > > - size_t datalen = (size_t)key->payload.data[big_key_len]; > > + struct big_key_payload *payload = to_big_key_payload(key->payload); > > > > - if (datalen > BIG_KEY_FILE_THRESHOLD) { > > - struct path *path = (struct path *)&key->payload.data[big_key_path]; > > + if (payload->length > BIG_KEY_FILE_THRESHOLD) { > > + struct path *path = &payload->path; > > > > path_put(path); > > path->mnt = NULL; > > path->dentry = NULL; > > } > > And similarly: > > if (payload->length > BIG_KEY_FILE_THRESHOLD) { > path_put(&payload->path); > payload->path.mnt = NULL; > payload->path.dentry = NULL; > } I will respin. Thanks! -Kees [1] https://lore.kernel.org/lkml/20170508214324.GA124468@beast/ [2] https://lore.kernel.org/lkml/20170528081249.GD22193@infradead.org/ -- Kees Cook