* [syzbot] upstream boot error: WARNING in netlink_ack
@ 2022-10-04 8:27 syzbot
2022-10-04 8:33 ` Dmitry Vyukov
0 siblings, 1 reply; 9+ messages in thread
From: syzbot @ 2022-10-04 8:27 UTC (permalink / raw)
To: bpf, davem, edumazet, fw, harshit.m.mogalapalli, kuba,
linux-kernel, netdev, pabeni, syzkaller-bugs
Hello,
syzbot found the following issue on:
HEAD commit: 725737e7c21d Merge tag 'statx-dioalign-for-linus' of git:/..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=10257034880000
kernel config: https://syzkaller.appspot.com/x/.config?x=486af5e221f55835
dashboard link: https://syzkaller.appspot.com/bug?extid=3a080099974c271cd7e9
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+3a080099974c271cd7e9@syzkaller.appspotmail.com
------------[ cut here ]------------
memcpy: detected field-spanning write (size 28) of single field "&errmsg->msg" at net/netlink/af_netlink.c:2447 (size 16)
WARNING: CPU: 3 PID: 3351 at net/netlink/af_netlink.c:2447 netlink_ack+0x8ac/0xb10 net/netlink/af_netlink.c:2447
Modules linked in:
CPU: 3 PID: 3351 Comm: dhcpcd Not tainted 6.0.0-syzkaller-00593-g725737e7c21d #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-2 04/01/2014
RIP: 0010:netlink_ack+0x8ac/0xb10 net/netlink/af_netlink.c:2447
Code: fa ff ff e8 36 c3 e5 f9 b9 10 00 00 00 4c 89 ee 48 c7 c2 20 3f fb 8a 48 c7 c7 80 3f fb 8a c6 05 e8 98 34 06 01 e8 26 77 a6 01 <0f> 0b e9 3a fa ff ff 41 be 00 01 00 00 41 bd 14 00 00 00 e9 ea fd
RSP: 0018:ffffc900220e7758 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff88801a798a80 RCX: 0000000000000000
RDX: ffff8880151c0180 RSI: ffffffff81611cb8 RDI: fffff5200441cedd
RBP: ffff88801ed850c0 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000080000000 R11: 0000000000000000 R12: 0000000000000000
R13: 000000000000001c R14: ffff88801ec8e400 R15: ffff88801ec8e414
FS: 00007faef0af8740(0000) GS:ffff88802cb00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fff6adbe000 CR3: 0000000027683000 CR4: 0000000000150ee0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
netlink_rcv_skb+0x33d/0x420 net/netlink/af_netlink.c:2507
genl_rcv+0x24/0x40 net/netlink/genetlink.c:803
netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline]
netlink_unicast+0x543/0x7f0 net/netlink/af_netlink.c:1345
netlink_sendmsg+0x917/0xe10 net/netlink/af_netlink.c:1921
sock_sendmsg_nosec net/socket.c:714 [inline]
sock_sendmsg+0xcf/0x120 net/socket.c:734
____sys_sendmsg+0x712/0x8c0 net/socket.c:2482
___sys_sendmsg+0x110/0x1b0 net/socket.c:2536
__sys_sendmsg+0xf3/0x1c0 net/socket.c:2565
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7faef0bf0163
Code: 64 89 02 48 c7 c0 ff ff ff ff eb b7 66 2e 0f 1f 84 00 00 00 00 00 90 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 55 c3 0f 1f 40 00 48 83 ec 28 89 54 24 1c 48
RSP: 002b:00007fff6adbdc48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007faef0bf0163
RDX: 0000000000000000 RSI: 00007fff6adbdc90 RDI: 0000000000000010
RBP: 00007fff6adc1ed8 R08: 0000000000000000 R09: 0000000000000000
R10: 00007faef0c6ffc0 R11: 0000000000000246 R12: 0000000000000010
R13: 00007fff6adc1cf0 R14: 0000000000000000 R15: 000055e5ebce0290
</TASK>
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [syzbot] upstream boot error: WARNING in netlink_ack
2022-10-04 8:27 [syzbot] upstream boot error: WARNING in netlink_ack syzbot
@ 2022-10-04 8:33 ` Dmitry Vyukov
2022-10-04 14:36 ` Kees Cook
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Vyukov @ 2022-10-04 8:33 UTC (permalink / raw)
To: syzbot
Cc: bpf, davem, edumazet, fw, harshit.m.mogalapalli, kuba,
linux-kernel, netdev, pabeni, syzkaller-bugs, Kees Cook,
linux-hardening
On Tue, 4 Oct 2022 at 10:27, syzbot
<syzbot+3a080099974c271cd7e9@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 725737e7c21d Merge tag 'statx-dioalign-for-linus' of git:/..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=10257034880000
> kernel config: https://syzkaller.appspot.com/x/.config?x=486af5e221f55835
> dashboard link: https://syzkaller.appspot.com/bug?extid=3a080099974c271cd7e9
> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+3a080099974c271cd7e9@syzkaller.appspotmail.com
+linux-hardening
> ------------[ cut here ]------------
> memcpy: detected field-spanning write (size 28) of single field "&errmsg->msg" at net/netlink/af_netlink.c:2447 (size 16)
> WARNING: CPU: 3 PID: 3351 at net/netlink/af_netlink.c:2447 netlink_ack+0x8ac/0xb10 net/netlink/af_netlink.c:2447
> Modules linked in:
> CPU: 3 PID: 3351 Comm: dhcpcd Not tainted 6.0.0-syzkaller-00593-g725737e7c21d #0
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-2 04/01/2014
> RIP: 0010:netlink_ack+0x8ac/0xb10 net/netlink/af_netlink.c:2447
> Code: fa ff ff e8 36 c3 e5 f9 b9 10 00 00 00 4c 89 ee 48 c7 c2 20 3f fb 8a 48 c7 c7 80 3f fb 8a c6 05 e8 98 34 06 01 e8 26 77 a6 01 <0f> 0b e9 3a fa ff ff 41 be 00 01 00 00 41 bd 14 00 00 00 e9 ea fd
> RSP: 0018:ffffc900220e7758 EFLAGS: 00010282
> RAX: 0000000000000000 RBX: ffff88801a798a80 RCX: 0000000000000000
> RDX: ffff8880151c0180 RSI: ffffffff81611cb8 RDI: fffff5200441cedd
> RBP: ffff88801ed850c0 R08: 0000000000000005 R09: 0000000000000000
> R10: 0000000080000000 R11: 0000000000000000 R12: 0000000000000000
> R13: 000000000000001c R14: ffff88801ec8e400 R15: ffff88801ec8e414
> FS: 00007faef0af8740(0000) GS:ffff88802cb00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fff6adbe000 CR3: 0000000027683000 CR4: 0000000000150ee0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> netlink_rcv_skb+0x33d/0x420 net/netlink/af_netlink.c:2507
> genl_rcv+0x24/0x40 net/netlink/genetlink.c:803
> netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline]
> netlink_unicast+0x543/0x7f0 net/netlink/af_netlink.c:1345
> netlink_sendmsg+0x917/0xe10 net/netlink/af_netlink.c:1921
> sock_sendmsg_nosec net/socket.c:714 [inline]
> sock_sendmsg+0xcf/0x120 net/socket.c:734
> ____sys_sendmsg+0x712/0x8c0 net/socket.c:2482
> ___sys_sendmsg+0x110/0x1b0 net/socket.c:2536
> __sys_sendmsg+0xf3/0x1c0 net/socket.c:2565
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> RIP: 0033:0x7faef0bf0163
> Code: 64 89 02 48 c7 c0 ff ff ff ff eb b7 66 2e 0f 1f 84 00 00 00 00 00 90 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 55 c3 0f 1f 40 00 48 83 ec 28 89 54 24 1c 48
> RSP: 002b:00007fff6adbdc48 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007faef0bf0163
> RDX: 0000000000000000 RSI: 00007fff6adbdc90 RDI: 0000000000000010
> RBP: 00007fff6adc1ed8 R08: 0000000000000000 R09: 0000000000000000
> R10: 00007faef0c6ffc0 R11: 0000000000000246 R12: 0000000000000010
> R13: 00007fff6adc1cf0 R14: 0000000000000000 R15: 000055e5ebce0290
> </TASK>
>
>
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/000000000000a793cc05ea313b87%40google.com.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [syzbot] upstream boot error: WARNING in netlink_ack
2022-10-04 8:33 ` Dmitry Vyukov
@ 2022-10-04 14:36 ` Kees Cook
2022-10-04 17:42 ` Jakub Kicinski
0 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2022-10-04 14:36 UTC (permalink / raw)
To: Dmitry Vyukov, syzbot
Cc: bpf, davem, edumazet, fw, harshit.m.mogalapalli, kuba,
linux-kernel, netdev, pabeni, syzkaller-bugs, linux-hardening
On October 4, 2022 1:33:30 AM PDT, Dmitry Vyukov <dvyukov@google.com> wrote:
>On Tue, 4 Oct 2022 at 10:27, syzbot
><syzbot+3a080099974c271cd7e9@syzkaller.appspotmail.com> wrote:
>>
>> Hello,
>>
>> syzbot found the following issue on:
>>
>> HEAD commit: 725737e7c21d Merge tag 'statx-dioalign-for-linus' of git:/..
>> git tree: upstream
>> console output: https://syzkaller.appspot.com/x/log.txt?x=10257034880000
>> kernel config: https://syzkaller.appspot.com/x/.config?x=486af5e221f55835
>> dashboard link: https://syzkaller.appspot.com/bug?extid=3a080099974c271cd7e9
>> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
>>
>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
>> Reported-by: syzbot+3a080099974c271cd7e9@syzkaller.appspotmail.com
>
>+linux-hardening
>
>> ------------[ cut here ]------------
>> memcpy: detected field-spanning write (size 28) of single field "&errmsg->msg" at net/netlink/af_netlink.c:2447 (size 16)
This is fixed in the pending netdev tree coming for the merge window.
--
Kees Cook
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [syzbot] upstream boot error: WARNING in netlink_ack
2022-10-04 14:36 ` Kees Cook
@ 2022-10-04 17:42 ` Jakub Kicinski
2022-10-04 23:40 ` Kees Cook
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2022-10-04 17:42 UTC (permalink / raw)
To: Kees Cook
Cc: Dmitry Vyukov, syzbot, bpf, davem, edumazet, fw,
harshit.m.mogalapalli, linux-kernel, netdev, pabeni,
syzkaller-bugs, linux-hardening
On Tue, 04 Oct 2022 07:36:55 -0700 Kees Cook wrote:
> This is fixed in the pending netdev tree coming for the merge window.
This has been weighing on my conscience a little, I don't like how we
still depend on putting one length in the skb and then using a
different one for the actual memcpy(). How would you feel about this
patch on top (untested):
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 4418b1981e31..6ad671441dff 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -931,6 +931,29 @@ static inline struct nlmsghdr *nlmsg_put(struct sk_buff *skb, u32 portid, u32 se
return __nlmsg_put(skb, portid, seq, type, payload, flags);
}
+/**
+ * nlmsg_append - Add more data to a nlmsg in a skb
+ * @skb: socket buffer to store message in
+ * @nlh: message header
+ * @payload: length of message payload
+ *
+ * Append data to an existing nlmsg, used when constructing a message
+ * with multiple fixed-format headers (which is rare).
+ * Returns NULL if the tailroom of the skb is insufficient to store
+ * the extra payload.
+ */
+static inline void *nlmsg_append(struct sk_buff *skb, struct nlmsghdr *nlh,
+ u32 size)
+{
+ if (unlikely(skb_tailroom(skb) < NLMSG_ALIGN(size)))
+ return NULL;
+
+ if (!__builtin_constant_p(size) || NLMSG_ALIGN(size) - size != 0)
+ memset(skb_tail_pointer(skb) + size, 0,
+ NLMSG_ALIGN(size) - size);
+ return __skb_put(NLMSG_ALIGN(size));
+}
+
/**
* nlmsg_put_answer - Add a new callback based netlink message to an skb
* @skb: socket buffer to store message in
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index a662e8a5ff84..bb3d855d1f57 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2488,19 +2488,28 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
flags |= NLM_F_ACK_TLVS;
skb = nlmsg_new(payload + tlvlen, GFP_KERNEL);
- if (!skb) {
- NETLINK_CB(in_skb).sk->sk_err = ENOBUFS;
- sk_error_report(NETLINK_CB(in_skb).sk);
- return;
- }
+ if (!skb)
+ goto err_bad_put;
rep = nlmsg_put(skb, NETLINK_CB(in_skb).portid, nlh->nlmsg_seq,
- NLMSG_ERROR, payload, flags);
+ NLMSG_ERROR, sizeof(*errmsg), flags);
+ if (!rep)
+ goto err_bad_put;
errmsg = nlmsg_data(rep);
errmsg->error = err;
- unsafe_memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg)
- ? nlh->nlmsg_len : sizeof(*nlh),
- /* Bounds checked by the skb layer. */);
+ memcpy(&errmsg->msg, nlh, sizeof(*nlh));
+
+ if (!(flags & NLM_F_CAPPED)) {
+ size_t data_len = nlh->nlmsg_len - sizeof(*nlh);
+ void *data;
+
+ data = nlmsg_append(skb, rep, data_len);
+ if (!data)
+ goto err_bad_put;
+
+ /* the nlh + 1 is probably going to make you unhappy? */
+ memcpy(data, nlh + 1, data_len);
+ }
if (tlvlen)
netlink_ack_tlv_fill(in_skb, skb, nlh, err, extack);
@@ -2508,6 +2517,12 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
nlmsg_end(skb, rep);
nlmsg_unicast(in_skb->sk, skb, NETLINK_CB(in_skb).portid);
+
+ return;
+
+err_bad_put:
+ NETLINK_CB(in_skb).sk->sk_err = ENOBUFS;
+ sk_error_report(NETLINK_CB(in_skb).sk);
}
EXPORT_SYMBOL(netlink_ack);
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [syzbot] upstream boot error: WARNING in netlink_ack
2022-10-04 17:42 ` Jakub Kicinski
@ 2022-10-04 23:40 ` Kees Cook
2022-10-05 0:04 ` Jakub Kicinski
2022-10-05 0:28 ` [RFC] netlink: split up copies in the ack construction Jakub Kicinski
0 siblings, 2 replies; 9+ messages in thread
From: Kees Cook @ 2022-10-04 23:40 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Dmitry Vyukov, syzbot, bpf, davem, edumazet, fw,
harshit.m.mogalapalli, linux-kernel, netdev, pabeni,
syzkaller-bugs, linux-hardening
On Tue, Oct 04, 2022 at 10:42:53AM -0700, Jakub Kicinski wrote:
> On Tue, 04 Oct 2022 07:36:55 -0700 Kees Cook wrote:
> > This is fixed in the pending netdev tree coming for the merge window.
>
> This has been weighing on my conscience a little, I don't like how we
> still depend on putting one length in the skb and then using a
> different one for the actual memcpy(). How would you feel about this
> patch on top (untested):
tl;dr: yes, I like it. Please add a nlmsg_contents member. :)
Rambling below...
>
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 4418b1981e31..6ad671441dff 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -931,6 +931,29 @@ static inline struct nlmsghdr *nlmsg_put(struct sk_buff *skb, u32 portid, u32 se
> return __nlmsg_put(skb, portid, seq, type, payload, flags);
> }
>
> +/**
> + * nlmsg_append - Add more data to a nlmsg in a skb
> + * @skb: socket buffer to store message in
> + * @nlh: message header
> + * @payload: length of message payload
> + *
> + * Append data to an existing nlmsg, used when constructing a message
> + * with multiple fixed-format headers (which is rare).
> + * Returns NULL if the tailroom of the skb is insufficient to store
> + * the extra payload.
> + */
> +static inline void *nlmsg_append(struct sk_buff *skb, struct nlmsghdr *nlh,
nlh not needed here?
> + u32 size)
> +{
> + if (unlikely(skb_tailroom(skb) < NLMSG_ALIGN(size)))
> + return NULL;
> +
> + if (!__builtin_constant_p(size) || NLMSG_ALIGN(size) - size != 0)
why does a fixed size mean no memset?
> + memset(skb_tail_pointer(skb) + size, 0,
> + NLMSG_ALIGN(size) - size);
> + return __skb_put(NLMSG_ALIGN(size));
> +}
> +
> /**
> * nlmsg_put_answer - Add a new callback based netlink message to an skb
> * @skb: socket buffer to store message in
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index a662e8a5ff84..bb3d855d1f57 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2488,19 +2488,28 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
> flags |= NLM_F_ACK_TLVS;
>
> skb = nlmsg_new(payload + tlvlen, GFP_KERNEL);
> - if (!skb) {
> - NETLINK_CB(in_skb).sk->sk_err = ENOBUFS;
> - sk_error_report(NETLINK_CB(in_skb).sk);
> - return;
> - }
> + if (!skb)
> + goto err_bad_put;
>
> rep = nlmsg_put(skb, NETLINK_CB(in_skb).portid, nlh->nlmsg_seq,
> - NLMSG_ERROR, payload, flags);
> + NLMSG_ERROR, sizeof(*errmsg), flags);
> + if (!rep)
> + goto err_bad_put;
> errmsg = nlmsg_data(rep);
> errmsg->error = err;
> - unsafe_memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg)
> - ? nlh->nlmsg_len : sizeof(*nlh),
> - /* Bounds checked by the skb layer. */);
> + memcpy(&errmsg->msg, nlh, sizeof(*nlh));
> +
> + if (!(flags & NLM_F_CAPPED)) {
Should it test this flag, or test if the sizes show the need for "extra"
payload length?
I always found the progression of sizes here to be confusing. "payload"
starts as sizeof(*errmsg), and gets nlmsg_len(nlh) added but only when if
"(err && !(nlk->flags & NETLINK_F_CAP_ACK)" was true. Why is
nlmsg_len(nlh) _wrong_ if the rest of its contents are correct? If this
was "0" in the other state, the logic would just be:
nlh_bytes = nlmsg_len(nlh);
total = sizeof(*errmsg);
total += nlh_bytes;
total += tlvlen;
and:
nlmsg_new(total, ...);
... nlmsg_put(..., sizeof(*errmsg), ...);
...
errmsg->error = err;
errmsg->nlh = *nlh;
if (nlh_bytes) {
data = nlmsg_append(..., nlh_bytes), ...);
...
memcpy(data, nlh->nlmsg_contents, nlh_bytes);
}
> + size_t data_len = nlh->nlmsg_len - sizeof(*nlh);
I think data_len here is also "payload - sizeof(*errmsg)"? So if it's >0,
we need to append the nlh contents.
> + void *data;
> +
> + data = nlmsg_append(skb, rep, data_len);
> + if (!data)
> + goto err_bad_put;
> +
> + /* the nlh + 1 is probably going to make you unhappy? */
Right, the compiler may think it is an object no larger than sizeof(*nlh).
My earliest attempt at changes here introduced a flex-array for the
contents, and split the memcpy:
https://lore.kernel.org/lkml/d7251d92-150b-5346-6237-52afc154bb00@rasmusvillemoes.dk/
which is basically the solution you have here, except it wasn't having
the nlmsg_*-helpers do the bounds checking.
> + memcpy(data, nlh + 1, data_len);
So with the struct nlmsghdr::nlmsg_contents member, this becomes:
memcpy(data, nlh->nlmsg_contents, data_len);
--
Kees Cook
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [syzbot] upstream boot error: WARNING in netlink_ack
2022-10-04 23:40 ` Kees Cook
@ 2022-10-05 0:04 ` Jakub Kicinski
2022-10-05 0:23 ` Jakub Kicinski
2022-10-05 0:28 ` [RFC] netlink: split up copies in the ack construction Jakub Kicinski
1 sibling, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2022-10-05 0:04 UTC (permalink / raw)
To: Kees Cook
Cc: Dmitry Vyukov, syzbot, bpf, davem, edumazet, fw,
harshit.m.mogalapalli, linux-kernel, netdev, pabeni,
syzkaller-bugs, linux-hardening
On Tue, 4 Oct 2022 16:40:32 -0700 Kees Cook wrote:
> On Tue, Oct 04, 2022 at 10:42:53AM -0700, Jakub Kicinski wrote:
> > This has been weighing on my conscience a little, I don't like how we
> > still depend on putting one length in the skb and then using a
> > different one for the actual memcpy(). How would you feel about this
> > patch on top (untested):
>
> tl;dr: yes, I like it. Please add a nlmsg_contents member. :)
Can do, but you'll need to tell me how..
__DECLARE_FLEX_ARRAY(char, nlmsg_contents)
?
> > + u32 size)
> > +{
> > + if (unlikely(skb_tailroom(skb) < NLMSG_ALIGN(size)))
> > + return NULL;
> > +
> > + if (!__builtin_constant_p(size) || NLMSG_ALIGN(size) - size != 0)
>
> why does a fixed size mean no memset?
Copy and paste, it seems to originate from:
0c19b0adb8dd ("netlink: avoid memset of 0 bytes sparse warning")
Any idea why sparse would not like empty memsets?
> > rep = nlmsg_put(skb, NETLINK_CB(in_skb).portid, nlh->nlmsg_seq,
> > - NLMSG_ERROR, payload, flags);
> > + NLMSG_ERROR, sizeof(*errmsg), flags);
> > + if (!rep)
> > + goto err_bad_put;
> > errmsg = nlmsg_data(rep);
> > errmsg->error = err;
> > - unsafe_memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg)
> > - ? nlh->nlmsg_len : sizeof(*nlh),
> > - /* Bounds checked by the skb layer. */);
> > + memcpy(&errmsg->msg, nlh, sizeof(*nlh));
> > +
> > + if (!(flags & NLM_F_CAPPED)) {
>
> Should it test this flag, or test if the sizes show the need for "extra"
> payload length?
>
> I always found the progression of sizes here to be confusing. "payload"
> starts as sizeof(*errmsg), and gets nlmsg_len(nlh) added but only when if
> "(err && !(nlk->flags & NETLINK_F_CAP_ACK)" was true.
struct nlmsgerr is one of the least badly documented structs we have in
netlink so let me start with a copy & paste:
struct nlmsgerr {
int error;
struct nlmsghdr msg;
/*
* followed by the message contents unless NETLINK_CAP_ACK was set
* or the ACK indicates success (error == 0)
* message length is aligned with NLMSG_ALIGN()
*/
/*
* followed by TLVs defined in enum nlmsgerr_attrs
* if NETLINK_EXT_ACK was set
*/
};
*Why* that's the behavior - 🤷
> Why is nlmsg_len(nlh) _wrong_ if the rest of its contents are
> correct?
This is an ack message, to be clear, doesn't mean anything was wrong.
It just carries errno.
> If this was "0" in the other state, the logic would just be:
>
> nlh_bytes = nlmsg_len(nlh);
> total = sizeof(*errmsg);
> total += nlh_bytes;
> total += tlvlen;
>
> and:
>
> nlmsg_new(total, ...);
> ... nlmsg_put(..., sizeof(*errmsg), ...);
> ...
> errmsg->error = err;
> errmsg->nlh = *nlh;
> if (nlh_bytes) {
> data = nlmsg_append(..., nlh_bytes), ...);
> ...
> memcpy(data, nlh->nlmsg_contents, nlh_bytes);
> }
>
> > + size_t data_len = nlh->nlmsg_len - sizeof(*nlh);
>
> I think data_len here is also "payload - sizeof(*errmsg)"? So if it's
> >0, we need to append the nlh contents.
I was trying to avoid using payload in case it has overflown :S
> > + void *data;
> > +
> > + data = nlmsg_append(skb, rep, data_len);
> > + if (!data)
> > + goto err_bad_put;
> > +
> > + /* the nlh + 1 is probably going to make you
> > unhappy? */
>
> Right, the compiler may think it is an object no larger than
> sizeof(*nlh). My earliest attempt at changes here introduced a
> flex-array for the contents, and split the memcpy:
> https://lore.kernel.org/lkml/d7251d92-150b-5346-6237-52afc154bb00@rasmusvillemoes.dk/
> which is basically the solution you have here, except it wasn't having
> the nlmsg_*-helpers do the bounds checking.
>
> > + memcpy(data, nlh + 1, data_len);
>
> So with the struct nlmsghdr::nlmsg_contents member, this becomes:
>
> memcpy(data, nlh->nlmsg_contents, data_len);
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [syzbot] upstream boot error: WARNING in netlink_ack
2022-10-05 0:04 ` Jakub Kicinski
@ 2022-10-05 0:23 ` Jakub Kicinski
0 siblings, 0 replies; 9+ messages in thread
From: Jakub Kicinski @ 2022-10-05 0:23 UTC (permalink / raw)
To: Kees Cook
Cc: Dmitry Vyukov, syzbot, bpf, davem, edumazet, fw,
harshit.m.mogalapalli, linux-kernel, netdev, pabeni,
syzkaller-bugs, linux-hardening
On Tue, 4 Oct 2022 17:04:00 -0700 Jakub Kicinski wrote:
> > why does a fixed size mean no memset?
>
> Copy and paste, it seems to originate from:
>
> 0c19b0adb8dd ("netlink: avoid memset of 0 bytes sparse warning")
>
> Any idea why sparse would not like empty memsets?
Google answers is. Let me test if sparse still wants the workaround.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC] netlink: split up copies in the ack construction
2022-10-04 23:40 ` Kees Cook
2022-10-05 0:04 ` Jakub Kicinski
@ 2022-10-05 0:28 ` Jakub Kicinski
2022-10-05 3:03 ` Kees Cook
1 sibling, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2022-10-05 0:28 UTC (permalink / raw)
To: Kees Cook; +Cc: netdev, linux-hardening, Jakub Kicinski
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
include/net/netlink.h | 21 +++++++++++++++++++++
include/uapi/linux/netlink.h | 2 ++
net/netlink/af_netlink.c | 30 +++++++++++++++++++++---------
3 files changed, 44 insertions(+), 9 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 4418b1981e31..46c40fabd2b5 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -931,6 +931,27 @@ static inline struct nlmsghdr *nlmsg_put(struct sk_buff *skb, u32 portid, u32 se
return __nlmsg_put(skb, portid, seq, type, payload, flags);
}
+/**
+ * nlmsg_append - Add more data to a nlmsg in a skb
+ * @skb: socket buffer to store message in
+ * @payload: length of message payload
+ *
+ * Append data to an existing nlmsg, used when constructing a message
+ * with multiple fixed-format headers (which is rare).
+ * Returns NULL if the tailroom of the skb is insufficient to store
+ * the extra payload.
+ */
+static inline void *nlmsg_append(struct sk_buff *skb, u32 size)
+{
+ if (unlikely(skb_tailroom(skb) < NLMSG_ALIGN(size)))
+ return NULL;
+
+ if (NLMSG_ALIGN(size) - size)
+ memset(skb_tail_pointer(skb) + size, 0,
+ NLMSG_ALIGN(size) - size);
+ return __skb_put(skb, NLMSG_ALIGN(size));
+}
+
/**
* nlmsg_put_answer - Add a new callback based netlink message to an skb
* @skb: socket buffer to store message in
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index e2ae82e3f9f7..fba3ca8152fa 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -48,6 +48,7 @@ struct sockaddr_nl {
* @nlmsg_flags: Additional flags
* @nlmsg_seq: Sequence number
* @nlmsg_pid: Sending process port ID
+ * @nlmsg_data: Message payload
*/
struct nlmsghdr {
__u32 nlmsg_len;
@@ -55,6 +56,7 @@ struct nlmsghdr {
__u16 nlmsg_flags;
__u32 nlmsg_seq;
__u32 nlmsg_pid;
+ __DECLARE_FLEX_ARRAY(char, nlmsg_data);
};
/* Flags values */
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index a662e8a5ff84..f8c94454b916 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2488,19 +2488,25 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
flags |= NLM_F_ACK_TLVS;
skb = nlmsg_new(payload + tlvlen, GFP_KERNEL);
- if (!skb) {
- NETLINK_CB(in_skb).sk->sk_err = ENOBUFS;
- sk_error_report(NETLINK_CB(in_skb).sk);
- return;
- }
+ if (!skb)
+ goto err_bad_put;
rep = nlmsg_put(skb, NETLINK_CB(in_skb).portid, nlh->nlmsg_seq,
- NLMSG_ERROR, payload, flags);
+ NLMSG_ERROR, sizeof(*errmsg), flags);
+ if (!rep)
+ goto err_bad_put;
errmsg = nlmsg_data(rep);
errmsg->error = err;
- unsafe_memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg)
- ? nlh->nlmsg_len : sizeof(*nlh),
- /* Bounds checked by the skb layer. */);
+ errmsg->msg = *nlh;
+
+ if (!(flags & NLM_F_CAPPED)) {
+ if (!nlmsg_append(skb, nlmsg_len(nlh)))
+ goto err_bad_put;
+
+ /* the nlh + 1 is probably going to make you unhappy? */
+ memcpy(errmsg->msg.nlmsg_data, nlh->nlmsg_data,
+ nlmsg_len(nlh));
+ }
if (tlvlen)
netlink_ack_tlv_fill(in_skb, skb, nlh, err, extack);
@@ -2508,6 +2514,12 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
nlmsg_end(skb, rep);
nlmsg_unicast(in_skb->sk, skb, NETLINK_CB(in_skb).portid);
+
+ return;
+
+err_bad_put:
+ NETLINK_CB(in_skb).sk->sk_err = ENOBUFS;
+ sk_error_report(NETLINK_CB(in_skb).sk);
}
EXPORT_SYMBOL(netlink_ack);
--
2.37.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC] netlink: split up copies in the ack construction
2022-10-05 0:28 ` [RFC] netlink: split up copies in the ack construction Jakub Kicinski
@ 2022-10-05 3:03 ` Kees Cook
0 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2022-10-05 3:03 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, linux-hardening
On Tue, Oct 04, 2022 at 05:28:14PM -0700, Jakub Kicinski wrote:
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> include/net/netlink.h | 21 +++++++++++++++++++++
> include/uapi/linux/netlink.h | 2 ++
> net/netlink/af_netlink.c | 30 +++++++++++++++++++++---------
> 3 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 4418b1981e31..46c40fabd2b5 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -931,6 +931,27 @@ static inline struct nlmsghdr *nlmsg_put(struct sk_buff *skb, u32 portid, u32 se
> return __nlmsg_put(skb, portid, seq, type, payload, flags);
> }
>
> +/**
> + * nlmsg_append - Add more data to a nlmsg in a skb
> + * @skb: socket buffer to store message in
> + * @payload: length of message payload
> + *
> + * Append data to an existing nlmsg, used when constructing a message
> + * with multiple fixed-format headers (which is rare).
> + * Returns NULL if the tailroom of the skb is insufficient to store
> + * the extra payload.
> + */
> +static inline void *nlmsg_append(struct sk_buff *skb, u32 size)
> +{
> + if (unlikely(skb_tailroom(skb) < NLMSG_ALIGN(size)))
> + return NULL;
> +
> + if (NLMSG_ALIGN(size) - size)
> + memset(skb_tail_pointer(skb) + size, 0,
> + NLMSG_ALIGN(size) - size);
> + return __skb_put(skb, NLMSG_ALIGN(size));
> +}
> +
> /**
> * nlmsg_put_answer - Add a new callback based netlink message to an skb
> * @skb: socket buffer to store message in
> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> index e2ae82e3f9f7..fba3ca8152fa 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -48,6 +48,7 @@ struct sockaddr_nl {
> * @nlmsg_flags: Additional flags
> * @nlmsg_seq: Sequence number
> * @nlmsg_pid: Sending process port ID
> + * @nlmsg_data: Message payload
> */
> struct nlmsghdr {
> __u32 nlmsg_len;
> @@ -55,6 +56,7 @@ struct nlmsghdr {
> __u16 nlmsg_flags;
> __u32 nlmsg_seq;
> __u32 nlmsg_pid;
> + __DECLARE_FLEX_ARRAY(char, nlmsg_data);
Since the flex array isn't part of a union, it can just be declared
"normally":
__u8 nlmsg_data[];
I'd also suggest u8 (rather than signed char) because compilers hate us,
and I've been burned too many times by having char arrays do stupid
things with the signed bit.
> };
>
> /* Flags values */
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index a662e8a5ff84..f8c94454b916 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2488,19 +2488,25 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
> flags |= NLM_F_ACK_TLVS;
>
> skb = nlmsg_new(payload + tlvlen, GFP_KERNEL);
> - if (!skb) {
> - NETLINK_CB(in_skb).sk->sk_err = ENOBUFS;
> - sk_error_report(NETLINK_CB(in_skb).sk);
> - return;
> - }
> + if (!skb)
> + goto err_bad_put;
>
> rep = nlmsg_put(skb, NETLINK_CB(in_skb).portid, nlh->nlmsg_seq,
> - NLMSG_ERROR, payload, flags);
> + NLMSG_ERROR, sizeof(*errmsg), flags);
> + if (!rep)
> + goto err_bad_put;
> errmsg = nlmsg_data(rep);
> errmsg->error = err;
> - unsafe_memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg)
> - ? nlh->nlmsg_len : sizeof(*nlh),
> - /* Bounds checked by the skb layer. */);
> + errmsg->msg = *nlh;
> +
> + if (!(flags & NLM_F_CAPPED)) {
> + if (!nlmsg_append(skb, nlmsg_len(nlh)))
> + goto err_bad_put;
> +
> + /* the nlh + 1 is probably going to make you unhappy? */
> + memcpy(errmsg->msg.nlmsg_data, nlh->nlmsg_data,
> + nlmsg_len(nlh));
> + }
>
> if (tlvlen)
> netlink_ack_tlv_fill(in_skb, skb, nlh, err, extack);
> @@ -2508,6 +2514,12 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
> nlmsg_end(skb, rep);
>
> nlmsg_unicast(in_skb->sk, skb, NETLINK_CB(in_skb).portid);
> +
> + return;
> +
> +err_bad_put:
> + NETLINK_CB(in_skb).sk->sk_err = ENOBUFS;
> + sk_error_report(NETLINK_CB(in_skb).sk);
> }
> EXPORT_SYMBOL(netlink_ack);
The rest looks great! :) I suspect you'll want to do close to the same
conversion in net/netfilter/ipset/ip_set_core.c in call_ad() too.
--
Kees Cook
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-10-05 3:03 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-04 8:27 [syzbot] upstream boot error: WARNING in netlink_ack syzbot
2022-10-04 8:33 ` Dmitry Vyukov
2022-10-04 14:36 ` Kees Cook
2022-10-04 17:42 ` Jakub Kicinski
2022-10-04 23:40 ` Kees Cook
2022-10-05 0:04 ` Jakub Kicinski
2022-10-05 0:23 ` Jakub Kicinski
2022-10-05 0:28 ` [RFC] netlink: split up copies in the ack construction Jakub Kicinski
2022-10-05 3:03 ` Kees Cook
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.