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 A3906C5320E for ; Mon, 19 Aug 2024 12:48:33 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1F4856B0088; Mon, 19 Aug 2024 08:48:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1A5186B0089; Mon, 19 Aug 2024 08:48:33 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 01DFA6B008A; Mon, 19 Aug 2024 08:48:32 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id D57D46B0088 for ; Mon, 19 Aug 2024 08:48:32 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 93E9D1C4DAA for ; Mon, 19 Aug 2024 12:48:32 +0000 (UTC) X-FDA: 82468973664.14.DD94807 Received: from mail-vk1-f172.google.com (mail-vk1-f172.google.com [209.85.221.172]) by imf08.hostedemail.com (Postfix) with ESMTP id B5661160037 for ; Mon, 19 Aug 2024 12:48:30 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=cZrhW5Gw; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf08.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.172 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1724071653; a=rsa-sha256; cv=none; b=wplyKoMJ9pPsMrU4p+dC6vrC8LvMkDFNMOvrGzJCgd+MJKTUOcmV6DZQYGGf8W1UDt4W2r mS+16idO2s0udSakaARGpYASxh5Zi3l/O3AOHP4BYQ0nsJyjwYRYXdxHteXppOEIjtmExR AEp202kKm62xd1EWkbia6zP2FHfwuiU= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=cZrhW5Gw; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf08.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.172 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1724071653; 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=WQXZdzAnQoHXoN4yZdckIUNNjAtYBDG0MxDRUv+ICko=; b=eE03ITF/dpz2Rn+n+pLljUr0u4O7RwgdJeTl/MJSMS730ZivKOkg8lI0N2JsiNTh+37hSu blu+Wq4WK0zj97AfReb46k+rKUllAyJYF5bzM54LhQ8NU1hvaHN48UxmoBw8fYJPvftoF/ hZWNo1bR7Y4J6t5hpcPA7bS1FZtQKvo= Received: by mail-vk1-f172.google.com with SMTP id 71dfb90a1353d-4f6ac477ff4so2354571e0c.3 for ; Mon, 19 Aug 2024 05:48:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1724071710; x=1724676510; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=WQXZdzAnQoHXoN4yZdckIUNNjAtYBDG0MxDRUv+ICko=; b=cZrhW5GwrFOJOQ2Ay+QGJGZ8xPzq+Gf9IKEXRVx9Ai67T3me6TBQQ8fZkuXm9Xjy2c 67iIIIDC9NczYl3BcrbYlvoxyMZiRy87hSr9IvMe3iHSiu5dAX9sIzNwUrLtbM/GGGMi O9rFfB6UOBt/PW6wx8oah8vtYd4SptJgNgKhoDGOF29YVzzn+vkhSv1RjPHZPTImUHWG ArT0kiHvke7B47wSzrs3JhozYQZo9CyOoEnnVcQWoX6o4yRL+w6W4OEDI19zdR1WJCSv pYeIPnc2c4nvMi1pMvpXdy9Y873ztwilzBOYuwCafqQwYNuXQIQN5CSAEJRHp/QOSvfi RvEA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1724071710; x=1724676510; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=WQXZdzAnQoHXoN4yZdckIUNNjAtYBDG0MxDRUv+ICko=; b=aIub36jMZHfIAp9aNdqB4BJXPQaNU12IsUCo3bkIdV8K/fgYjnhFsml8S38vNfrJxy P9qFJWGVBovAtVFzDtjw/HpkYGLBbW5pU1S1aLEjGrvf11P3vQFhwPVNMGJ22Kuq3Spk 8NCJmVyEvER6IKDzSxXzz/3Wcv1RZMuoGoGSqHwsWaUvuiu/7bPYvtF5+Y7NWbW4wXXT LFRdYDKrsotV+Lq6MHlBM8s3+bE4hmcrX9iayw027PSP5VScLbfctX0e9/7ScapRCPU+ BDWrtVXCNrvvesFZ7cYN8anYIgmskYGvIwq2qZLVp60caaRZc33/31e5jHMTZ3zGUlNj 66lw== X-Forwarded-Encrypted: i=1; AJvYcCWTbYE2Fc5g/xbfdPQJvCK6F19qhi4zvWP9c2lKdQxyXHh6AzZmkIOraBMlAny4/WXG47Ykspz8jwX784GjrvxXxbs= X-Gm-Message-State: AOJu0YxlgghB/+fc2ZcJDPmVF2S7Jp3DfiakPiAgUma5C50YbReVL/Zn +kyos1B1CkZYxAcvex5u7j7CwpGgpg/uFXZlHbHjJbEKDckJsv4Htq6YbD+CrqkhGZ3grHACT8y SsCYoPGguV16Lqz/o/LhOaU0js10= X-Google-Smtp-Source: AGHT+IF7//DmJ6IWybIFsN9Jr7uQaycyCdYWtKQaiK4iQFmy/g3uf+B2D0Ph/CT22DTUWSfYRky4+/igQ0JSVZj67kU= X-Received: by 2002:a05:6122:1685:b0:4f5:abe4:50e2 with SMTP id 71dfb90a1353d-4fc6c601202mr13525978e0c.6.1724071709603; Mon, 19 Aug 2024 05:48:29 -0700 (PDT) MIME-Version: 1.0 References: <20240817062449.21164-1-21cnbao@gmail.com> <20240817062449.21164-4-21cnbao@gmail.com> <5654b71c-1d9d-4c48-b28b-664662da8897@redhat.com> <416ac265-ced2-4f90-a347-0a256edf7fdf@redhat.com> <54a4619d-e826-465e-9a0f-0a8f37798e15@redhat.com> In-Reply-To: <54a4619d-e826-465e-9a0f-0a8f37798e15@redhat.com> From: Barry Song <21cnbao@gmail.com> Date: Tue, 20 Aug 2024 00:48:18 +1200 Message-ID: Subject: Re: [PATCH v3 3/4] mm: BUG_ON to avoid NULL deference while __GFP_NOFAIL fails To: David Hildenbrand Cc: akpm@linux-foundation.org, linux-mm@kvack.org, 42.hyeyoo@gmail.com, cl@linux.com, hailong.liu@oppo.com, hch@infradead.org, iamjoonsoo.kim@lge.com, mhocko@suse.com, penberg@kernel.org, rientjes@google.com, roman.gushchin@linux.dev, torvalds@linux-foundation.org, urezki@gmail.com, v-songbaohua@oppo.com, vbabka@suse.cz, virtualization@lists.linux.dev, Christoph Hellwig , Lorenzo Stoakes , Kees Cook , =?UTF-8?Q?Eugenio_P=C3=A9rez?= , Jason Wang , Maxime Coquelin , "Michael S. Tsirkin" , Xuan Zhuo Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: B5661160037 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: g8tr55nc4uh9mpcihoch6e5tph1g3wmx X-HE-Tag: 1724071710-901138 X-HE-Meta: U2FsdGVkX1/HU/1eC4nMw/w8SJhlkdwk7oVNtUrAXuMMepJfGXEXh4arlrJZ71JipZrVqzrESirJZlkfH+VlUHhpvhPRBQQDolD3mZ3sX4bGY7yFBu0QSdV0pMXCiXtPP3jpmvx9uMWjtXWJWCAxtbLFUZE+Ozq1tIpncbCT2t6kzv/VAqbYjN0tN3tP8Y7XkXdO4s0zhGL3H5EL4GMk+X7nesanC/kQo/b9avY3L6ZyjpMGtP1g7wxdOsyUvGmNNDKFcM51AKGo0X7avnuS7G2OtuBeqqURf0PMmLNm3oc4X5gzSpTvDGIX96mNlUU7DgOPRlLrOoHGgARmpgN2gkv/zQqWEA40/XQVejuSXeNXD9W/D3kpiQSzHFCeSCU7v3bO8TuOP9b8oW37MN6qDqVgu3FIrQ48lWBRn/e+oYfxZCE125AHQ2Js0/uTUGQVLrH2J60zirwE9w6gF6JA05YoZrLkjmpN2Nj0SADALM7ODYL5LB+s/TUrKlQSilf3Vrk+JxD/Lp6G9eCAcBmqfMmwtrTOo7fZIAlkgxWMfAeGPb3JNJSi9hX1pZfmrPm5jJ6HWPnMftAiXCz7RS4pZIp6bNH//OjMTyqF8iWH5UlsNiJUZC+Tr6glU2ijkcBKOJKz+LtVSV4iZw/4cQmOUAu3w3lAGYvRwn652hyFy5Qo8qP0kuCJsZZ9jmrSroPXCZZO9uiidL6i2Tg/ABm6WI1vh6tMZemz34v/6QlWAuRMe3+8/HlzrY0RwdPGW58VGd8uHkACc5KMwK3rUJ5eJ/s6JWJrnD/OTrIzN2GFDJVN2mdGmsNdVf1vdmIyJ50tJDIB8gtFh1UPljTvnB+3Bp86RLsl4A/rO8tdHeVyWhnL5a/SXPaPBzHJv0hA+qb6jywCSwk//3/kSAipgvHjvU4IxR6dMC3jaIGKMBxSUbBvstJRCKtHNqBlxpCWlKvcMhZAyWsvCUX5r3sbKJg gTViMSKf FSjB7UCiDPwdsVzhYzpbwt1lyAQ9r+qJN0VvPnhtu7pIqjcYBcaw/7HJPkkxoPI5RFRMM5n4tTz0SqEJzemw7AHSmWUSrU0O1Xf9i+xj9Q9KIOCAG7snDUONldySFJZGnQHMbdSZSnytZ0ZRj759u7CQjCvn/eXehrWc5WgDDGr02gOOsEqxm7SqvKyf1DwmFanci3N3hnDpeUrU5MFBLNIDQjDf47U2dVwBDdC3ooNA01quidXg9ovo2eXmNae8UgCoN+aiVL/Qd/bXK12cJEjmUGa5l/jgubfrLSaNfgEbIrdo21xTLGXrUp8eqcRNR98A7I95PKF0MoGLfDyF/JV89OsTQGcSMMPhm9WImWtrSmcXHMPF5/pPp5htdLBSYfkfAn85w3t94emBAjel9obOTCDAykSYgZdB5PlF78wFvc4KrRKaO6AgTHwJd8gR2HUUIW2vgz6iA9TiGyhBVM0BCowZwkCw1+0+OcBgot81agU8iBy+fZSnFdaFTYDxOlR73M6OZ+e5mxvUNvAqZVawLwEF7WDvKP+wgYJyLicxZhrwy5g+qmBjUAoEWR9/vFgwuMl9B9QwUUuf7r9i3E64JnUQH4INeuteKhY4+uHr6WHLIoMGT14AGzIe+LrMh7zdnVyOFDPChy8w/4nbWbIiTnCN/8cgocHb1GmV8FYqMnY3Fw098wuYzEpqviWipRBQijqVq/kFPOv2PQlQOXLC7GiUbyHK9EWXKE1q6qKOivQSfb3w7nsV1ipophsYOc+p8JPy5bexTrgJHh6SvQZOpIBSdqNTLZF7AJk+qLMWodQk0x4HzjAip+/sTSA8/e1p5tUq5M5cGZw5ZDDDdDwPrrhf1rv/Hek87w9AiHpzQtPfUE1F9U+VerurS8pULhiSV 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 Tue, Aug 20, 2024 at 12:33=E2=80=AFAM David Hildenbrand wrote: > > On 19.08.24 12:02, Barry Song wrote: > > On Mon, Aug 19, 2024 at 9:55=E2=80=AFPM David Hildenbrand wrote: > >> > >> On 19.08.24 11:47, Barry Song wrote: > >>> On Mon, Aug 19, 2024 at 9:43=E2=80=AFPM David Hildenbrand wrote: > >>>> > >>>> On 17.08.24 08:24, Barry Song wrote: > >>>>> From: Barry Song > >>>>> > >>>>> We have cases we still fail though callers might have __GFP_NOFAIL.= Since > >>>>> they don't check the return, we are exposed to the security risks f= or NULL > >>>>> deference. > >>>>> > >>>>> Though BUG_ON() is not encouraged by Linus, this is an unrecoverabl= e > >>>>> situation. > >>>>> > >>>>> Christoph Hellwig: > >>>>> The whole freaking point of __GFP_NOFAIL is that callers don't hand= le > >>>>> allocation failures. So in fact a straight BUG is the right thing > >>>>> here. > >>>>> > >>>>> Vlastimil Babka: > >>>>> It's just not a recoverable situation (WARN_ON is for recoverable > >>>>> situations). The caller cannot handle allocation failure and at the= same > >>>>> time asked for an impossible allocation. BUG_ON() is a guaranteed o= ops > >>>>> with stracktrace etc. We don't need to hope for the later NULL poin= ter > >>>>> dereference (which might if really unlucky happen from a different > >>>>> context where it's no longer obvious what lead to the allocation fa= iling). > >>>>> > >>>>> Michal Hocko: > >>>>> Linus tends to be against adding new BUG() calls unless the failure= is > >>>>> absolutely unrecoverable (e.g. corrupted data structures etc.). I a= m > >>>>> not sure how he would look at simply incorrect memory allocator usa= ge to > >>>>> blow up the kernel. Now the argument could be made that those failu= res > >>>>> could cause subtle memory corruptions or even be exploitable which = might > >>>>> be a sufficient reason to stop them early. > >>>>> > >>>>> Signed-off-by: Barry Song > >>>>> Reviewed-by: Christoph Hellwig > >>>>> Acked-by: Vlastimil Babka > >>>>> Acked-by: Michal Hocko > >>>>> Cc: Uladzislau Rezki (Sony) > >>>>> Cc: Lorenzo Stoakes > >>>>> Cc: Christoph Lameter > >>>>> Cc: Pekka Enberg > >>>>> Cc: David Rientjes > >>>>> Cc: Joonsoo Kim > >>>>> Cc: Roman Gushchin > >>>>> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com> > >>>>> Cc: Linus Torvalds > >>>>> Cc: Kees Cook > >>>>> Cc: "Eugenio P=C3=A9rez" > >>>>> Cc: Hailong.Liu > >>>>> Cc: Jason Wang > >>>>> Cc: Maxime Coquelin > >>>>> Cc: "Michael S. Tsirkin" > >>>>> Cc: Xuan Zhuo > >>>>> --- > >>>>> include/linux/slab.h | 4 +++- > >>>>> mm/page_alloc.c | 4 +++- > >>>>> mm/util.c | 1 + > >>>>> 3 files changed, 7 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/include/linux/slab.h b/include/linux/slab.h > >>>>> index c9cb42203183..4a4d1fdc2afe 100644 > >>>>> --- a/include/linux/slab.h > >>>>> +++ b/include/linux/slab.h > >>>>> @@ -827,8 +827,10 @@ kvmalloc_array_node_noprof(size_t n, size_t si= ze, gfp_t flags, int node) > >>>>> { > >>>>> size_t bytes; > >>>>> > >>>>> - if (unlikely(check_mul_overflow(n, size, &bytes))) > >>>>> + if (unlikely(check_mul_overflow(n, size, &bytes))) { > >>>>> + BUG_ON(flags & __GFP_NOFAIL); > >>>>> return NULL; > >>>>> + } > >>>>> > >>>>> return kvmalloc_node_noprof(bytes, flags, node); > >>>>> } > >>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c > >>>>> index 60742d057b05..d2c37f8f8d09 100644 > >>>>> --- a/mm/page_alloc.c > >>>>> +++ b/mm/page_alloc.c > >>>>> @@ -4668,8 +4668,10 @@ struct page *__alloc_pages_noprof(gfp_t gfp,= unsigned int order, > >>>>> * There are several places where we assume that the order = value is sane > >>>>> * so bail out early if the request is out of bound. > >>>>> */ > >>>>> - if (WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp)) > >>>>> + if (WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER, gfp)) { > >>>>> + BUG_ON(gfp & __GFP_NOFAIL); > >>>>> return NULL; > >>>>> + } > >>>>> > >>>>> gfp &=3D gfp_allowed_mask; > >>>>> /* > >>>>> diff --git a/mm/util.c b/mm/util.c > >>>>> index ac01925a4179..678c647b778f 100644 > >>>>> --- a/mm/util.c > >>>>> +++ b/mm/util.c > >>>>> @@ -667,6 +667,7 @@ void *__kvmalloc_node_noprof(DECL_BUCKET_PARAMS= (size, b), gfp_t flags, int node) > >>>>> > >>>>> /* Don't even allow crazy sizes */ > >>>>> if (unlikely(size > INT_MAX)) { > >>>>> + BUG_ON(flags & __GFP_NOFAIL); > >>>> > >>>> No new BUG_ON please. WARN_ON_ONCE() + recovery code might be suitab= le here. > >>> > >>> Hi David, > >>> WARN_ON_ONCE() might be fine but I don't see how it is possible to r= ecover. > >> > >> Just return NULL? "shit in shit out" :) ? > > > > Returning NULL is perfectly right if gfp doesn't include __GFP_NOFAIL, > > as it's the caller's responsibility to check the return value. However,= with > > __GFP_NOFAIL, users will directly dereference *(p + offset) even when > > p =3D=3D NULL. It is how __GFP_NOFAIL is supposed to work. > > If the caller is not supposed to pass that flag combination (shit in), > we are not obligated to give a reasonable result (shit out). > > My point is that we should let the caller (possibly?) crash -- the one > that did something that is wrong -- instead of forcing a crash using > BUG_ON in this code here. > > It should all be caught during testing either way. And if some OOT > module does something nasty, that's not our responsibility. > > BUG_ON is not a way to write assertions into the code. It seems there was a misunderstanding regarding the purpose of this change. we actually have many details in changelog. Its aim is not to write an assertion, but rather to prevent exposing a security vulnerability. Returning NULL doesn't necessarily crash the caller's process, p->field, *(p + offset) deference could be used by hackers to exploit the system. > > -- > Cheers, > > David / dhildenb > Thanks Barry