| Message ID | 2592945.1703376169@warthog.procyon.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | keys, dns: Fix missing size check of V1 server-list header | expand |
On Sun, Dec 24, 2023 at 12:02:49AM +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> Thanks. FWIIW, I prefer this approach where v1 and bin don't alias each other, and the scope of v1 is constrained to the block where it is used. Reviewed-by: Simon Horman <horms@kernel.org> ...
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 " 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
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
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
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 >
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
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 >
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;
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
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)