* Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header
2024-01-10 4:40 ` Pengfei Xu
@ 2024-01-10 5:19 ` Edward Adam Davis
2024-01-10 5:47 ` Pengfei Xu
2024-01-10 5:27 ` Pengfei Xu
2024-01-10 10:14 ` David Howells
2 siblings, 1 reply; 14+ messages in thread
From: Edward Adam Davis @ 2024-01-10 5:19 UTC (permalink / raw)
To: pengfei.xu
Cc: ceph-devel, davem, dhowells, eadavis, edumazet, heng.su, horms,
jaltman, jarkko, jlayton, keyrings, kuba, linux-afs, linux-cifs,
linux-fsdevel, linux-kernel, linux-nfs, marc.dionne,
markus.suvanto, netdev, pabeni, smfrench, torvalds, wang840925
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.
> "
>
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header
2024-01-10 5:19 ` Edward Adam Davis
@ 2024-01-10 5:47 ` Pengfei Xu
0 siblings, 0 replies; 14+ messages in thread
From: Pengfei Xu @ 2024-01-10 5:47 UTC (permalink / raw)
To: Edward Adam Davis
Cc: ceph-devel, davem, dhowells, edumazet, heng.su, horms, jaltman,
jarkko, jlayton, keyrings, kuba, linux-afs, linux-cifs,
linux-fsdevel, linux-kernel, linux-nfs, marc.dionne,
markus.suvanto, netdev, pabeni, smfrench, torvalds, wang840925
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header
2024-01-10 4:40 ` Pengfei Xu
2024-01-10 5:19 ` Edward Adam Davis
@ 2024-01-10 5:27 ` Pengfei Xu
2024-01-10 10:14 ` David Howells
2 siblings, 0 replies; 14+ messages in thread
From: Pengfei Xu @ 2024-01-10 5:27 UTC (permalink / raw)
To: David Howells, eadavis
Cc: Linus Torvalds, Simon Horman, Markus Suvanto, Jeffrey E Altman,
Marc Dionne, Wang Lei, Jeff Layton, Steve French, Jarkko Sakkinen,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-afs, keyrings, linux-cifs, linux-nfs, ceph-devel, netdev,
linux-fsdevel, linux-kernel, heng.su
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header
2024-01-10 4:40 ` Pengfei Xu
2024-01-10 5:19 ` Edward Adam Davis
2024-01-10 5:27 ` Pengfei Xu
@ 2024-01-10 10:14 ` David Howells
2024-01-10 11:06 ` Pengfei Xu
2 siblings, 1 reply; 14+ messages in thread
From: David Howells @ 2024-01-10 10:14 UTC (permalink / raw)
To: Pengfei Xu
Cc: dhowells, eadavis, Linus Torvalds, Simon Horman, Markus Suvanto,
Jeffrey E Altman, Marc Dionne, Wang Lei, Jeff Layton,
Steve French, Jarkko Sakkinen, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-afs, keyrings, linux-cifs,
linux-nfs, ceph-devel, netdev, linux-fsdevel, linux-kernel,
heng.su
Pengfei Xu <pengfei.xu@intel.com> wrote:
> 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)
> "
It should fail as the payload is actually invalid. The payload specifies a
version 1 format - and that requires a 6-byte header. The bug the patched
fixes is that whilst there is a length check for the basic 3-byte header,
there was no length check for the extended v1 header.
> After increased the dns_res_payload to 7 bytes(6 bytes was still failed),
The following doesn't work for you?
echo -n -e '\0\0\01\xff\0\0' | keyctl padd dns_resolver desc @p
David
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header
2024-01-10 10:14 ` David Howells
@ 2024-01-10 11:06 ` Pengfei Xu
2024-01-10 17:23 ` David Howells
0 siblings, 1 reply; 14+ messages in thread
From: Pengfei Xu @ 2024-01-10 11:06 UTC (permalink / raw)
To: David Howells
Cc: eadavis, Linus Torvalds, Simon Horman, Markus Suvanto,
Jeffrey E Altman, Marc Dionne, Wang Lei, Jeff Layton,
Steve French, Jarkko Sakkinen, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-afs, keyrings, linux-cifs,
linux-nfs, ceph-devel, netdev, linux-fsdevel, linux-kernel,
heng.su
On 2024-01-10 at 10:14:28 +0000, David Howells wrote:
> Pengfei Xu <pengfei.xu@intel.com> wrote:
>
> > 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)
> > "
>
> It should fail as the payload is actually invalid. The payload specifies a
> version 1 format - and that requires a 6-byte header. The bug the patched
> fixes is that whilst there is a length check for the basic 3-byte header,
> there was no length check for the extended v1 header.
Thanks for description!
>
> > After increased the dns_res_payload to 7 bytes(6 bytes was still failed),
>
> The following doesn't work for you?
>
> echo -n -e '\0\0\01\xff\0\0' | keyctl padd dns_resolver desc @p
I tried as follows, 6 bytes failed and 7 bytes passed:
"
# echo -n -e '\0\0\01\xff\0\0' | keyctl padd dns_resolver desc @p
add_key: Invalid argument
# echo -n -e '\0\0\01\xff\0\0\0' | keyctl padd dns_resolver desc @p
74678921
# uname -r
6.7.0-rc8
"
Thanks!
>
> David
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header
2024-01-10 11:06 ` Pengfei Xu
@ 2024-01-10 17:23 ` David Howells
2024-01-10 18:52 ` Linus Torvalds
0 siblings, 1 reply; 14+ messages in thread
From: David Howells @ 2024-01-10 17:23 UTC (permalink / raw)
To: Pengfei Xu
Cc: dhowells, eadavis, Linus Torvalds, Simon Horman, Markus Suvanto,
Jeffrey E Altman, Marc Dionne, Wang Lei, Jeff Layton,
Steve French, Jarkko Sakkinen, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-afs, keyrings, linux-cifs,
linux-nfs, ceph-devel, netdev, linux-fsdevel, linux-kernel,
heng.su
Meh. Does the attached fix it for you?
David
---
diff --git a/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index f18ca02aa95a..c42ddd85ff1f 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -104,7 +104,7 @@ dns_resolver_preparse(struct key_preparsed_payload *prep)
const struct dns_server_list_v1_header *v1;
/* It may be a server list. */
- if (datalen <= sizeof(*v1))
+ if (datalen < sizeof(*v1))
return -EINVAL;
v1 = (const struct dns_server_list_v1_header *)data;
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] keys, dns: Fix missing size check of V1 server-list header
2024-01-10 17:23 ` David Howells
@ 2024-01-10 18:52 ` Linus Torvalds
0 siblings, 0 replies; 14+ messages in thread
From: Linus Torvalds @ 2024-01-10 18:52 UTC (permalink / raw)
To: David Howells
Cc: Pengfei Xu, eadavis, Simon Horman, Markus Suvanto,
Jeffrey E Altman, Marc Dionne, Wang Lei, Jeff Layton,
Steve French, Jarkko Sakkinen, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-afs, keyrings, linux-cifs,
linux-nfs, ceph-devel, netdev, linux-fsdevel, linux-kernel,
heng.su
On Wed, 10 Jan 2024 at 09:23, David Howells <dhowells@redhat.com> wrote:
>
> Meh. Does the attached fix it for you?
Bah. Obvious fix is obvious.
Mind sending it as a proper patch with sign-off etc, and we'll get
this fixed and marked for stable.
Linus
^ permalink raw reply [flat|nested] 14+ messages in thread