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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 37D88C433F5 for ; Mon, 25 Oct 2021 22:30:08 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id BDE8E60E09 for ; Mon, 25 Oct 2021 22:30:07 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org BDE8E60E09 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id C2767940008; Mon, 25 Oct 2021 18:30:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BD742940007; Mon, 25 Oct 2021 18:30:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A77CF940008; Mon, 25 Oct 2021 18:30:06 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0211.hostedemail.com [216.40.44.211]) by kanga.kvack.org (Postfix) with ESMTP id 99C6E940007 for ; Mon, 25 Oct 2021 18:30:06 -0400 (EDT) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 1D00C2D38D for ; Mon, 25 Oct 2021 22:30:06 +0000 (UTC) X-FDA: 78736404012.29.06B9615 Received: from mail-pj1-f43.google.com (mail-pj1-f43.google.com [209.85.216.43]) by imf04.hostedemail.com (Postfix) with ESMTP id 00C5750000A1 for ; Mon, 25 Oct 2021 22:30:00 +0000 (UTC) Received: by mail-pj1-f43.google.com with SMTP id q2-20020a17090a2e0200b001a0fd4efd49so67558pjd.1 for ; Mon, 25 Oct 2021 15:30:05 -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=IddZQTyrCr8HT48PQm9m39l5JFYt46wr7uNMWEr0FH0=; b=IXTOoHtSl2NEkkG/zSTDmi8NbcF1s0dc235SPl6k0S6Wjg195c43mShxx0hBOH70zb 0SUW4sIJjngbhS964reoqzKYf+S7osgVm6qtIrCZHOKuKJvfml60NZaa3de3qhG3yr2I NVl+ObGA746xxAli4zvcpyUOdXEs1K5ZVZ0/8= 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=IddZQTyrCr8HT48PQm9m39l5JFYt46wr7uNMWEr0FH0=; b=EZiWk59a6/pmwUgJ44IVd29Ib/dQmeDOFftl8pOPNBuwybRwdCqswzbWYLPtS2QvtT oRhUksO/Kle3Qxjt25QqAZm2ddMM43ogZsbcSAySGuHqUrjW24IYYQZQKwI/68I9MTqB b8JQXuqz+vrvfvKHJ+ZhuInzqOcq6+ceaUZ2V3kKcBcV9nXfJl2tJS6uEPYWFDKXUARD 9wrt8g45YaQxVC4+sUGvTGqyqiUhSSYtL9klQr9sn4xeycv/bD53YLwvr/ndZ9O7lCmq 5nGmHSSFnFvK3mRHX6LC7R0Q0JUsnhUZqeRsgDAFkYaftrAL5VNRp+JWu8Tfg/i5aZU7 u74A== X-Gm-Message-State: AOAM53337XGVzbI5+P0q75gerLnSW9YP5zu0W6HgDtESaoiSvNLDBz1z r++BSyNkF5rO0nZ25k4pq6jzUQ== X-Google-Smtp-Source: ABdhPJyHf74fiyczGcPdRHcai8y8mFtM7Fvwl1YLbpP6dOqH3ElDxBOmPyPOdGGTCyMUPtaRRr9p0Q== X-Received: by 2002:a17:90a:e7cc:: with SMTP id kb12mr12801334pjb.176.1635201004421; Mon, 25 Oct 2021 15:30:04 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id x15sm1958414pfp.30.2021.10.25.15.30.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Oct 2021 15:30:04 -0700 (PDT) Date: Mon, 25 Oct 2021 15:30:03 -0700 From: Kees Cook To: Linus Torvalds Cc: Matthew Wilcox , Linux Kernel Mailing List , Linux-MM , Jordy Zomer , James Bottomley , Mike Rapoport , Andrew Morton Subject: Re: [PATCH] secretmem: Prevent secretmem_users from wrapping to zero Message-ID: <202110251438.1762406A5@keescook> References: <20211025181634.3889666-1-willy@infradead.org> <202110251225.D01841AE67@keescook> <202110251402.ADFA4D41BF@keescook> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 00C5750000A1 X-Stat-Signature: 14iqqiesxduwfxcwchd4s4r9w44mtn16 Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=IXTOoHtS; dmarc=pass (policy=none) header.from=chromium.org; spf=pass (imf04.hostedemail.com: domain of keescook@chromium.org designates 209.85.216.43 as permitted sender) smtp.mailfrom=keescook@chromium.org X-HE-Tag: 1635201000-201124 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: On Mon, Oct 25, 2021 at 02:17:58PM -0700, Linus Torvalds wrote: > On Mon, Oct 25, 2021 at 2:04 PM Kees Cook wrote: > > > > Is secretmem different? We're trying to count how many of these we have, > > this is a common pattern in, for example, the network code which does > > this kind of thing a lot. > > Yes, secretmem is different. Prefix: I'm fine with this being whatever; it's a corner case, so this reply is mainly about nailing down the rationales for future decisions. > A refcount being zero means that the data it referenced no longer exists. I don't disagree with this definition, but I would like to understand how some other use-cases fit into this. The case of basic allocated object memory lifetime reference counting we all agree on. What about the case of what I see that is more like a "shared resource usage count" where the shared resource doesn't necessarily disappear when we reach "no users"? i.e. there is some resource, and it starts its life with no one using it (count = 1). Then we have users declaring their use (count = 2) and later release (count = 1) of the resource. It's not really ever unallocated when users == 0 (count == 1), but we might use the usage counter for other things and don't want to let it go negative or ever increment from zero (in case zero is used for marking the resource unavailable forever). For example, protocols knowing if there are any sockets left open, crypto API usage counts, kernel module usage counts, etc. I don't see as clear a distinction between secretmem and the above examples. The question being answered is "how many users do I have?" and we want to make sure we don't end up with overflow or underflow given the (unfortunately) common case of reference counting kernel bugs. The fact that secretmem doesn't have its own allocation to free when it hits 0 seems like just an implementation detail of this particular resource usage counter. But, ignoring secretmem for a moment, what about a specific example from above: the module loader's refcnt atomic_t, which is actually maintaining an allocation (the module), and uses usage counts of 0 to mean "I am removing this module", and 1 is "I have no users", 2 is "1 user", etc. It seems like this should use refcount_t to me? > Stop arguing for garbage. It was wrong, just admit it. The semantics > for "refcount" is something else than what that code had. As a result, > it got reverted. I've applied Willy's patch that actually makes sense. > > Arguing for garbage in the name of "security" is still garbage. In > fact, it only causes confusion, and as such is likely to result in > problems - including security problems - later. Sure, and reasonable people can disagree about what is garbage. :) I see using a refcount_t here as a not unreasonable way to protect against potential future security problems if the scope of secretmem ever grows and more than just hibernation starts to care about this usage counter. > Because confusion about semantics is bad. And that was what that patch was. > > And I want to state - again - how dangerous this "refcounts are always > prefereable to atomics" mental model is. Refcounts are _not_ > fundamentally preferable to atomics. They are slower, bigger, and have > completely different semantics. I will push back on this; I don't think that's a fair assessment at all. For storage size, refcount_t is identical to atomic_t (refcount_t _is_ an atomic_t). But perhaps you meant code size? Yes, the refcount_t helpers are technically larger. But like speed, where refcount_t is also technically slower, neither size nor speed were so much changed that we kept around the routines that made it exactly the same speed and grew the instruction count by 1 (the original x86-specific refcount_t implementation was just as fast as atomic_t). I don't see speed nor size alone to be a good reason to say "don't use refcount_t". But yes, I agree about the different semantics: there are very specific memory ordering assumptions that tend to be more strict than atomic_t (which IMO actually makes it more suitable than atomic_t for shared usage counts). > So if something isn't a refcount, it damn well shouldn't use "refcount_t". Again, I don't disagree, but since it looked like a refcount to me, I'd like to understand what why we don't see this case the same way. Since I agree that secretmem is currently pretty iffy (nothing actually allocated to track, only system state), I'll ask a slightly different question: should the module loader use refcount_t? If not, why? -- Kees Cook