Received-SPF: pass (google.com: domain of linux-nfs+bounces-1005-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 10 Jan 2024 13:47:20 +0800
From: Pengfei Xu <pengfei.xu@intel.com>
To: Edward Adam Davis <eadavis@qq.com>
CC: <ceph-devel@vger.kernel.org>, <davem@davemloft.net>,
	<dhowells@redhat.com>, <edumazet@google.com>, <heng.su@intel.com>,
	<horms@kernel.org>, <jaltman@auristor.com>, <jarkko@kernel.org>,
	<jlayton@redhat.com>, <keyrings@vger.kernel.org>, <kuba@kernel.org>,
	<linux-afs@lists.infradead.org>, <linux-cifs@vger.kernel.org>,
	<linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-nfs@vger.kernel.org>, <marc.dionne@auristor.com>,
	<markus.suvanto@gmail.com>, <netdev@vger.kernel.org>, <pabeni@redhat.com>,
	<smfrench@gmail.com>, <torvalds@linux-foundation.org>, <wang840925@gmail.com>
Subject: Re: [PATCH] keys, dns: Fix missing size check of V1 server-list
 header
Message-ID: <ZZ4vaJMN2w/ilkR3@xpf.sh.intel.com>
References: <ZZ4fyY4r3rqgZL+4@xpf.sh.intel.com>
 <tencent_CF4FEF0D9B25A08DD7920E5D93DDBC194E07@qq.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <tencent_CF4FEF0D9B25A08DD7920E5D93DDBC194E07@qq.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?4Ply4IGCCwy+huPRVnOGftpyh2q+LybKRWrmGX/3mwYQivTc3oOBthLd5dUz?=
 =?us-ascii?Q?/0hsrFkHwfHm/Kx34ezSB1JUqTavwkfv8ZjW7iauVJQDjB1e9Hp11mSXdevI?=
 =?us-ascii?Q?3nCOnnl43VpvlS4MbEB4WGyZ4DUIubCM3nfSs1uLYY8+H+Wjo0wrMikeDk3W?=
 =?us-ascii?Q?+zla+EzdU0lIeEnby2+ejOHBMiFaSHsoy2+Xdz627Gv+l0DLMKx5f/T2ZiAI?=
 =?us-ascii?Q?srlFRK0QCts5UdY5ae227XqJ7ZrY2/bjf+Hzq+/cMc+QmJUaMiT11njvuCbc?=
 =?us-ascii?Q?4bF14DIj3hSE0FFd4Luhk7u8nIO7c0zvLAAFfPjJpCIVzkghJlBW8J9w+JDN?=
 =?us-ascii?Q?/RhS9rUvJ69zW5hOwF3+Palvmzsb+cr76J1LWIR8Ac2HDDSBdS2TUNUNuic8?=
 =?us-ascii?Q?ajTH35AFpLIoIdOfuN1ENd0STjONT7dhkjNN5vhvJZr32O4uwVZefHm3kDHt?=
 =?us-ascii?Q?2t2pgayxgs2OkbqylclsI91mmP1imMCBpy7xOSA/Tzpy6EHQ8nANt50mZuHi?=
 =?us-ascii?Q?E7hFWfXLaC6Unw+zQmVTD1W+oQ2Iq9jXL3h6skiiOy98HsDbPr4+qaNHG1cN?=
 =?us-ascii?Q?nZjROEOUSzcaB9AqFqzQUUGB+Hu9Vv0HlzQbcRwTjo8G+c4tzGYOSPsEOhWq?=
 =?us-ascii?Q?VfWvI/rsvAQfZ95izwPTyK/QZRVZB9UcUUBBThn49NyTUlmJ1jtLS9KonbCv?=
 =?us-ascii?Q?8zGcQZV9RacMgVmwDNiLgWhx/j40LmAzk/72IoWE178HVntGUDcNcaP4Hlua?=
 =?us-ascii?Q?TIbpnZpbNEo2Xa+fmWax5j0YVeRtBB2o5m/bUSQ7ap8hG+pDyXds7PN1HGV9?=
 =?us-ascii?Q?xlzCrYR8mMHPlksaxPE4BFcrjVqDIIW9DIoQ1Shosor/lIDZSqLdYIrNK1AL?=
 =?us-ascii?Q?CSaZBgE+CdEkBMNjGJ+UE5iBfU1kiDsJi38nNMl+S/pPL9eeiUqleYKUkI+I?=
 =?us-ascii?Q?9C0QE6C4c9HURNm4dTpbpWYxWEBbdIR7Djp+eyfvrW0/xY4+f4FV/JLickgZ?=
 =?us-ascii?Q?xG8a+KXvceWi3nNUuB+716E7HVCzS2EFjznzantkFrSwAOmhQBGrBbJaYBDg?=
 =?us-ascii?Q?Anu6WT4QXMqA4sNjHdZuUFjvEFskDqy+60YHNY0jip/cbN7oCHTcmwCvlD2Y?=
 =?us-ascii?Q?o989yanN7ivJKBKpwy/K8KZWboedrXy9ekq5KSZCrmWsusZqzgPKPeTegV2N?=
 =?us-ascii?Q?tlAXP+CU9Mqcwczn0lvweHphywD/9kXiPL7a4nP6OlwlUV8buPrZ5jCoYOMe?=
 =?us-ascii?Q?vW3xqrtatVtRqZERFTQrF0t+HOpbdD6vzWg9MgY2/CbF5t9osM43L7kHKYmn?=
 =?us-ascii?Q?PNnpOwl6ex1x7cNGkvbHOQf/gqV5pZVgZiWZEPf6U14b79x4NVS+5xrTXveG?=
 =?us-ascii?Q?EHkQO/LN22uX4VXk/Ds0R+USkzM57bUcUHq1jZVDcTVQ7g12pawxBMHfid2I?=
 =?us-ascii?Q?8v+j+/8S9Z7WdMVCrFPB2A2YS7LAs+T25XHds4gOzyPAPIAyXSKOrA0f5uXJ?=
 =?us-ascii?Q?u462KJxrU4UvNqYANIjexVMfe6R+PUi8L6zbDE1/Ecjrg0KQeaCiCFBdSMEI?=
 =?us-ascii?Q?cG4nSHeaePqy14dJJcu6Z2ImYkbdBtmJgl0C9Oxu?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9dfa95f0-763b-4a1a-f530-08dc11a0634e
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB4839.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2024 05:52:53.9413
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: QMQ6MYGugQR/ZDOewSlucsXKg+oOx1r+XmepAOlp5/qWIN6N4vDsLi8ec3Se5N8+rE91PIYrPeheOX0DXEtfBA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB6127
X-OriginatorOrg: intel.com

On 2024-01-10 at 13:19:49 +0800, Edward Adam Davis wrote:
> On Wed, 10 Jan 2024 12:40:41 +0800, Pengfei Xu wrote:
> > > Hi Linus, Edward,
> > >
> > > Here's Linus's patch dressed up with a commit message.  I would marginally
> > > prefer just to insert the missing size check, but I'm also fine with Linus's
> > > approach for now until we have different content types or newer versions.
> > >
> > > Note that I'm not sure whether I should require Linus's S-o-b since he made
> > > modifications or whether I should use a Codeveloped-by line for him.
> > >
> > > David
> > > ---
> > > From: Edward Adam Davis <eadavis@qq.com>
> > >
> > > keys, dns: Fix missing size check of V1 server-list header
> > >
> > > The dns_resolver_preparse() function has a check on the size of the payload
> > > for the basic header of the binary-style payload, but is missing a check
> > > for the size of the V1 server-list payload header after determining that's
> > > what we've been given.
> > >
> > > Fix this by getting rid of the the pointer to the basic header and just
> > > assuming that we have a V1 server-list payload and moving the V1 server
> > > list pointer inside the if-statement.  Dealing with other types and
> > > versions can be left for when such have been defined.
> > >
> > > This can be tested by doing the following with KASAN enabled:
> > >
> > >         echo -n -e '\x0\x0\x1\x2' | keyctl padd dns_resolver foo @p
> > >
> > > and produces an oops like the following:
> > >
> > >         BUG: KASAN: slab-out-of-bounds in dns_resolver_preparse+0xc9f/0xd60 net/dns_resolver/dns_key.c:127
> > >         Read of size 1 at addr ffff888028894084 by task syz-executor265/5069
> > >         ...
> > >         Call Trace:
> > >          <TASK>
> > >          __dump_stack lib/dump_stack.c:88 [inline]
> > >          dump_stack_lvl+0xd9/0x1b0 lib/dump_stack.c:106
> > >          print_address_description mm/kasan/report.c:377 [inline]
> > >          print_report+0xc3/0x620 mm/kasan/report.c:488
> > >          kasan_report+0xd9/0x110 mm/kasan/report.c:601
> > >          dns_resolver_preparse+0xc9f/0xd60 net/dns_resolver/dns_key.c:127
> > >          __key_create_or_update+0x453/0xdf0 security/keys/key.c:842
> > >          key_create_or_update+0x42/0x50 security/keys/key.c:1007
> > >          __do_sys_add_key+0x29c/0x450 security/keys/keyctl.c:134
> > >          do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > >          do_syscall_64+0x40/0x110 arch/x86/entry/common.c:83
> > >          entry_SYSCALL_64_after_hwframe+0x62/0x6a
> > >
> > > This patch was originally by Edward Adam Davis, but was modified by Linus.
> > >
> > > Fixes: b946001d3bb1 ("keys, dns: Allow key types (eg. DNS) to be reclaimed immediately on expiry")
> > > Reported-and-tested-by: syzbot+94bbb75204a05da3d89f@syzkaller.appspotmail.com
> > > Link: https://lore.kernel.org/r/0000000000009b39bc060c73e209@google.com/
> > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > Signed-off-by: David Howells <dhowells@redhat.com>
> > > Tested-by: David Howells <dhowells@redhat.com>
> > > cc: Edward Adam Davis <eadavis@qq.com>
> > > cc: Simon Horman <horms@kernel.org>
> > > cc: Linus Torvalds <torvalds@linux-foundation.org>
> > > cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > cc: Jeffrey E Altman <jaltman@auristor.com>
> > > cc: Wang Lei <wang840925@gmail.com>
> > > cc: Jeff Layton <jlayton@redhat.com>
> > > cc: Steve French <sfrench@us.ibm.com>
> > > cc: Marc Dionne <marc.dionne@auristor.com>
> > > cc: "David S. Miller" <davem@davemloft.net>
> > > cc: Eric Dumazet <edumazet@google.com>
> > > cc: Jakub Kicinski <kuba@kernel.org>
> > > cc: Paolo Abeni <pabeni@redhat.com>
> > > cc: linux-afs@lists.infradead.org
> > > cc: linux-cifs@vger.kernel.org
> > > cc: linux-nfs@vger.kernel.org
> > > cc: ceph-devel@vger.kernel.org
> > > cc: keyrings@vger.kernel.org
> > > cc: netdev@vger.kernel.org
> > > ---
> > >  net/dns_resolver/dns_key.c |   19 +++++++++----------
> > >  1 file changed, 9 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
> > > index 2a6d363763a2..f18ca02aa95a 100644
> > > --- a/net/dns_resolver/dns_key.c
> > > +++ b/net/dns_resolver/dns_key.c
> > > @@ -91,8 +91,6 @@ const struct cred *dns_resolver_cache;
> > >  static int
> > >  dns_resolver_preparse(struct key_preparsed_payload *prep)
> > >  {
> > > -	const struct dns_server_list_v1_header *v1;
> > > -	const struct dns_payload_header *bin;
> > >  	struct user_key_payload *upayload;
> > >  	unsigned long derrno;
> > >  	int ret;
> > > @@ -103,27 +101,28 @@ dns_resolver_preparse(struct key_preparsed_payload *prep)
> > >  		return -EINVAL;
> > >
> > >  	if (data[0] == 0) {
> > > +		const struct dns_server_list_v1_header *v1;
> > > +
> > >  		/* It may be a server list. */
> > > -		if (datalen <= sizeof(*bin))
> > > +		if (datalen <= sizeof(*v1))
> > >  			return -EINVAL;
> > >
> > > -		bin = (const struct dns_payload_header *)data;
> > > -		kenter("[%u,%u],%u", bin->content, bin->version, datalen);
> > > -		if (bin->content != DNS_PAYLOAD_IS_SERVER_LIST) {
> > > +		v1 = (const struct dns_server_list_v1_header *)data;
> > > +		kenter("[%u,%u],%u", v1->hdr.content, v1->hdr.version, datalen);
> > > +		if (v1->hdr.content != DNS_PAYLOAD_IS_SERVER_LIST) {
> > >  			pr_warn_ratelimited(
> > >  				"dns_resolver: Unsupported content type (%u)\n",
> > > -				bin->content);
> > > +				v1->hdr.content);
> > >  			return -EINVAL;
> > >  		}
> > >
> > > -		if (bin->version != 1) {
> > > +		if (v1->hdr.version != 1) {
> > >  			pr_warn_ratelimited(
> > >  				"dns_resolver: Unsupported server list version (%u)\n",
> > > -				bin->version);
> > > +				v1->hdr.version);
> > >  			return -EINVAL;
> > >  		}
> > >
> > > -		v1 = (const struct dns_server_list_v1_header *)bin;
> > >  		if ((v1->status != DNS_LOOKUP_GOOD &&
> > >  		     v1->status != DNS_LOOKUP_GOOD_WITH_BAD)) {
> > >  			if (prep->expiry == TIME64_MAX)
> > >
> > 
> > Hi Edward and kernel experts,
> > 
> >   Above patch(upstream commit: 1997b3cb4217b09) seems causing a keyctl05 case
> > to fail in LTP:
> > https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/keyctl/keyctl05.c
> > 
> > It could be reproduced on a bare metal platform.
> > Kconfig: https://raw.githubusercontent.com/xupengfe/kconfig_diff/main/config_v6.7-rc8
> > Seems general kconfig could reproduce this issue.
> > 
> >   Bisected info between v6.7-rc7(keyctl05 passed) and v6.7-rc8(keyctl05 failed)
> > is in attached.
> > 
> > keyctl05 failed in add_key with type "dns_resolver" syscall step tracked
> > by strace:
> > "
> > [pid 863107] add_key("dns_resolver", "desc", "\0\0\1\377\0", 5, KEY_SPEC_SESSION_KEYRING <unfinished ...>
> > [pid 863106] <... alarm resumed>)       = 30
> > [pid 863107] <... add_key resumed>)     = -1 EINVAL (Invalid argument)
> The reason for the failure of add_key() is that the length of the incoming data
> is 5, which is less than sizeof(*v1), so keyctl05.c failed.
> Suggest modifying keyctl05.c to increase the length of the incoming data to 6 
> bytes or more.

Thanks for your suggestion!
dns_server_list_v1_header struct is 6 u8 data instead of previous bin.

After increased the dns_res_payload to 7 bytes(6 bytes was still failed),
keyctl05 could be passed.
"
static char dns_res_payload[] = { 0x00, 0x00, 0x01, 0xff, 0x00, 0x00, 0x00 };
"

I will improve the case in LTP.

Thanks!

> > "
> > 
> > Passed behavior in v6.7-rc7 kernel:
> > "
> > [pid  6726] add_key("dns_resolver", "desc", "\0\0\1\377\0", 5, KEY_SPEC_SESSION_KEYRING <unfinished ...>
> > [pid  6725] rt_sigreturn({mask=[]})     = 61
> > [pid  6726] <... add_key resumed>)      = 1029222644
> > "
> > 
> > Do you mind to take a look for above issue?
> Edward,
> BR
>