Received-SPF: pass (google.com: domain of linux-nfs+bounces-1004-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 10 Jan 2024 13:27:25 +0800
From: Pengfei Xu <pengfei.xu@intel.com>
To: David Howells <dhowells@redhat.com>, <eadavis@qq.com>
CC: Linus Torvalds <torvalds@linux-foundation.org>, Simon Horman
	<horms@kernel.org>, Markus Suvanto <markus.suvanto@gmail.com>, "Jeffrey E
 Altman" <jaltman@auristor.com>, Marc Dionne <marc.dionne@auristor.com>, "Wang
 Lei" <wang840925@gmail.com>, Jeff Layton <jlayton@redhat.com>, Steve French
	<smfrench@gmail.com>, Jarkko Sakkinen <jarkko@kernel.org>, "David S. Miller"
	<davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski
	<kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	<linux-afs@lists.infradead.org>, <keyrings@vger.kernel.org>,
	<linux-cifs@vger.kernel.org>, <linux-nfs@vger.kernel.org>,
	<ceph-devel@vger.kernel.org>, <netdev@vger.kernel.org>,
	<linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<heng.su@intel.com>
Subject: Re: [PATCH] keys, dns: Fix missing size check of V1 server-list
 header
Message-ID: <ZZ4qvVD5oQOxBLvB@xpf.sh.intel.com>
References: <CAHk-=wgJz36ZE66_8gXjP_TofkkugXBZEpTr_Dtc_JANsH1SEw@mail.gmail.com>
 <1843374.1703172614@warthog.procyon.org.uk>
 <20231223172858.GI201037@kernel.org>
 <2592945.1703376169@warthog.procyon.org.uk>
 <ZZ4fyY4r3rqgZL+4@xpf.sh.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZZ4fyY4r3rqgZL+4@xpf.sh.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?QJl5Uz72x0aQx03hNLWlruwrbGdOs/zChGnlxAeouVcqa0IayR+heMVqJHNB?=
 =?us-ascii?Q?V8kr0I/LKJAKBK292FFDuhIjd5LeXu9onXQNOxpLnn7s+kut+MRzHMZuh0XT?=
 =?us-ascii?Q?Z0EDOh3yW7NOs6doVx2+EnOgG7mJZEoRXVIti5XmBQSVnOK/FwX6Ti4oTrnj?=
 =?us-ascii?Q?RQPFUPHNAULgTbO8js2mUs4NLNhqdfn5wm9xKhyxOYdIp86z0mUkp3oRhtSc?=
 =?us-ascii?Q?KZ6hBveFbMNs98WKbgbaQ9QvPvqc7+780vg9n5HsXHf7FqJRknukRzqPl42f?=
 =?us-ascii?Q?L/jcw1um8tPVtlON1tYHiLsxz3istbaNTod6IKLfc03/91TlDFMYxRrh3547?=
 =?us-ascii?Q?CksO0Tc4sLOTnDT+eSJC6rYgtHFSXqGuXTaj8Ce7RM2kM3GgIv+XMAfDfQGj?=
 =?us-ascii?Q?lZ9mFb78HCIFrfHxVwOY0pKG2AVX44e8QV0osS/IerzAs9n8vnNPU4Pl6GDI?=
 =?us-ascii?Q?Qbudgw0qdvnwtujp8SvSg8u6Ex6jpniBNusS9REqPBdHr1FIXWWRbycppEJe?=
 =?us-ascii?Q?/FjKuy9LSr6ak5x5gkeb96XLKe7Nc1ls+5HgeGxHpcukDui2RgMqw/TADMyX?=
 =?us-ascii?Q?3YiVNZPodZSzCBbaKbJO2pyFuWiwjgOTTO7sX/eeingomdHvUrmB8FZv+5e9?=
 =?us-ascii?Q?8+rnJZ3RXb0OGY/M/7YkN83CPi9ZE16lJ+/PaEY4nzdslNNXDFdXq51wwZuL?=
 =?us-ascii?Q?LICEzrRw0lQWNr3oCz/u7z2FzgEAQzzSrJhrd82QWHa1dtDttlmR8ZknHr4I?=
 =?us-ascii?Q?yO8lvc3QDmPZgd13hU3LUIr9xURZrMbvbn1j35cQSG2asxT2OrL6SM3koDdL?=
 =?us-ascii?Q?UGWfjPFGNi8mJQ8nb51i586N/ggDlGbqppmK4wAlP1KxD7Xu2H5+GTROWqbv?=
 =?us-ascii?Q?SqBt6n0EAQS0buvZDp1vMdQ7s64//HDc3TuIXWYbHFDVxtt57e/PuBJkmmcR?=
 =?us-ascii?Q?VcvZk3r5A1TpOi8IVc4YLzmnHUELdWSBXXyz4YXi4hPBUWPxJQsI9iD1YzQ5?=
 =?us-ascii?Q?RccCJk5hjPzNBG6I85MD6Smw8rhQh7MiTLC06dgkEay0tIN0+ThWmT4u4SOf?=
 =?us-ascii?Q?u0Kn77YA5/od6ofohHafxMCPF7Vfvsd4wct1QfoqGB6q3HntU9f1xKrgbdN6?=
 =?us-ascii?Q?o+6q8/W83l1Mog/FGie1hU2MkI7xG31oeJ/cEUxLKtTbP0Z0I3Idz09G0BPN?=
 =?us-ascii?Q?0lTj4aXhcyWMCZ5IPL0potbjHntnTavW+oUfNkO/8jIl7F76GetNRmVK+YxS?=
 =?us-ascii?Q?BwCihEzp/5wnKzxN/Yqy3d94eo6fqevbA4HyqWbuj7EW+HjyY2mqJgrDtT42?=
 =?us-ascii?Q?2NflZCfWEFDoacASdcINK7IwsfRd1huBMuPLe3DcYlXAi7ZKDAMh/J4P1bzI?=
 =?us-ascii?Q?GFL+xWUhKhvUFrZgalhHnEzx1n17HqMiy6IkRG06IlZdUr591fLSiH9+uwXd?=
 =?us-ascii?Q?Nibpyem8P+SsrB4glvqneDDWkJU9n1bifez9BODcO6GbtLVVTcipEDWba7WO?=
 =?us-ascii?Q?YG6HqSZn5vYPBVABHibz3FN3s7eoX+UQFUC6uTt30yHny8TOgfp34cZYkAEH?=
 =?us-ascii?Q?mVpGVrcFm1cgNbLvz3g3XowYsJ0xaUcrEmobKoNy?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8db03b8e-680b-4e11-f347-08dc119d9cde
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:33:02.0333
 (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: LOBBct22WPiY8oVlYhjHOpdAtDUWMz39pjzoeAynsDr6mDVVgXNqTmTamOzkSZpV0VHk/gc38Dk/YYWthH/rKA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW6PR11MB8392
X-OriginatorOrg: intel.com

On 2024-01-10 at 12:40:41 +0800, Pengfei Xu wrote:
> On 2023-12-24 at 00:02:49 +0000, David Howells 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)
> "
> 
> 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
> "

Sorry, updated more keyctl05 failed in add_key with type "dns_resolver"
syscall step tracked by strace:
"
[pid 863107] getppid()                  = 863106
[pid 863107] kill(863106, SIGUSR1)      = 0
[pid 863106] <... wait4 resumed>0x7ffc5ec94858, 0, NULL) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
[pid 863107] keyctl(KEYCTL_JOIN_SESSION_KEYRING, NULL <unfinished ...>
[pid 863106] --- SIGUSR1 {si_signo=SIGUSR1, si_code=SI_USER, si_pid=863107, si_uid=0} ---
[pid 863107] <... keyctl resumed>)      = 512571383
[pid 863106] alarm(30 <unfinished ...>

[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)
[pid 863106] rt_sigreturn({mask=[]} <unfinished ...>
[pid 863107] write(2, "keyctl05.c:114: TFAIL: unexpecte"..., 79keyctl05.c:114: TFAIL: unexpected error adding 'dns_resolver' key: EINVAL (22)
 <unfinished ...>
[pid 863106] <... rt_sigreturn resumed>) = 61
[pid 863107] <... write resumed>)       = 79
"

Passed behavior in v6.7-rc7 kernel:
"
[pid  6726] getppid()                   = 6725
[pid  6726] kill(6725, SIGUSR1 <unfinished ...>
[pid  6725] <... wait4 resumed>0x7ffc6cad1c68, 0, NULL) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
[pid  6726] <... kill resumed>)         = 0
[pid  6725] --- SIGUSR1 {si_signo=SIGUSR1, si_code=SI_USER, si_pid=6726, si_uid=0} ---
[pid  6726] keyctl(KEYCTL_JOIN_SESSION_KEYRING, NULL <unfinished ...>
[pid  6725] alarm(30 <unfinished ...>
[pid  6726] <... keyctl resumed>)       = 713868472
[pid  6725] <... alarm resumed>)        = 30

[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?
> 
> Best Regards,
> Thanks!

> git bisect start
> # status: waiting for both good and bad commits
> # good: [861deac3b092f37b2c5e6871732f3e11486f7082] Linux 6.7-rc7
> git bisect good 861deac3b092f37b2c5e6871732f3e11486f7082
> # status: waiting for bad commit, 1 good commit known
> # bad: [610a9b8f49fbcf1100716370d3b5f6f884a2835a] Linux 6.7-rc8
> git bisect bad 610a9b8f49fbcf1100716370d3b5f6f884a2835a
> # good: [861deac3b092f37b2c5e6871732f3e11486f7082] Linux 6.7-rc7
> git bisect good 861deac3b092f37b2c5e6871732f3e11486f7082
> # bad: [505e701c0b2cfa9e34811020829759b7663a604c] Merge tag 'kbuild-fixes-v6.7-2' of git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild
> git bisect bad 505e701c0b2cfa9e34811020829759b7663a604c
> # good: [1803d0c5ee1a3bbee23db2336e21add067824f02] mailmap: add an old address for Naoya Horiguchi
> git bisect good 1803d0c5ee1a3bbee23db2336e21add067824f02
> # bad: [eeec2599630ac1ac03db98f3ba976975c72a1427] Merge tag 'bcachefs-2023-12-27' of https://evilpiepirate.org/git/bcachefs
> git bisect bad eeec2599630ac1ac03db98f3ba976975c72a1427
> # bad: [f5837722ffecbbedf1b1dbab072a063565f0dad1] Merge tag 'mm-hotfixes-stable-2023-12-27-15-00' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> git bisect bad f5837722ffecbbedf1b1dbab072a063565f0dad1
> # good: [b8e0792449928943c15d1af9f63816911d139267] virtio_blk: fix snprintf truncation compiler warning
> git bisect good b8e0792449928943c15d1af9f63816911d139267
> # bad: [1997b3cb4217b09e49659b634c94da47f0340409] keys, dns: Fix missing size check of V1 server-list header
> git bisect bad 1997b3cb4217b09e49659b634c94da47f0340409
> # good: [fbafc3e621c3f4ded43720fdb1d6ce1728ec664e] Merge tag 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost
> git bisect good fbafc3e621c3f4ded43720fdb1d6ce1728ec664e
> # first bad commit: [1997b3cb4217b09e49659b634c94da47f0340409] keys, dns: Fix missing size check of V1 server-list header