Re: [PATCH v2] decnet: dn_rtmsg: Improve input length sanitization in dnrmg_receive_user_skb
From: Mateusz JurczykDate: Wed, 7 Jun 2017 16:41:57 +0200 > On Wed, Jun 7, 2017 at 4:18 PM, Florian Westphal wrote: >> Mateusz Jurczyk wrote: >>> Verify that the length of the socket buffer is sufficient to cover the >>> nlmsghdr structure before accessing the nlh->nlmsg_len field for further >>> input sanitization. If the client only supplies 1-3 bytes of data in >>> sk_buff, then nlh->nlmsg_len remains partially uninitialized and >>> contains leftover memory from the corresponding kernel allocation. >>> Operating on such data may result in indeterminate evaluation of the >>> nlmsg_len < sizeof(*nlh) expression. >>> >>> The bug was discovered by a runtime instrumentation designed to detect >>> use of uninitialized memory in the kernel. The patch prevents this and >>> other similar tools (e.g. KMSAN) from flagging this behavior in the future. >> >> Instead of changing all the internal users wouldn't it be better >> to add this check once in netlink_unicast_kernel? >> > > Perhaps. I must admit I'm not very familiar with this code > area/interface, so I preferred to fix the few specific cases instead > of submitting a general patch, which might have some unexpected side > effects, e.g. behavior different from one of the internal clients etc. > > If you think one check in netlink_unicast_kernel is a better way to do > it, I'm happy to implement it like that. Until we decide to add the check to netlink_unicast_kernel(), I'm applying this and queueing it up for -stable. Thanks.
Re: [PATCH v2] decnet: dn_rtmsg: Improve input length sanitization in dnrmg_receive_user_skb
On Wed, Jun 7, 2017 at 4:18 PM, Florian Westphalwrote: > Mateusz Jurczyk wrote: >> Verify that the length of the socket buffer is sufficient to cover the >> nlmsghdr structure before accessing the nlh->nlmsg_len field for further >> input sanitization. If the client only supplies 1-3 bytes of data in >> sk_buff, then nlh->nlmsg_len remains partially uninitialized and >> contains leftover memory from the corresponding kernel allocation. >> Operating on such data may result in indeterminate evaluation of the >> nlmsg_len < sizeof(*nlh) expression. >> >> The bug was discovered by a runtime instrumentation designed to detect >> use of uninitialized memory in the kernel. The patch prevents this and >> other similar tools (e.g. KMSAN) from flagging this behavior in the future. > > Instead of changing all the internal users wouldn't it be better > to add this check once in netlink_unicast_kernel? > Perhaps. I must admit I'm not very familiar with this code area/interface, so I preferred to fix the few specific cases instead of submitting a general patch, which might have some unexpected side effects, e.g. behavior different from one of the internal clients etc. If you think one check in netlink_unicast_kernel is a better way to do it, I'm happy to implement it like that. Thanks, Mateusz
Re: [PATCH v2] decnet: dn_rtmsg: Improve input length sanitization in dnrmg_receive_user_skb
Mateusz Jurczykwrote: > Verify that the length of the socket buffer is sufficient to cover the > nlmsghdr structure before accessing the nlh->nlmsg_len field for further > input sanitization. If the client only supplies 1-3 bytes of data in > sk_buff, then nlh->nlmsg_len remains partially uninitialized and > contains leftover memory from the corresponding kernel allocation. > Operating on such data may result in indeterminate evaluation of the > nlmsg_len < sizeof(*nlh) expression. > > The bug was discovered by a runtime instrumentation designed to detect > use of uninitialized memory in the kernel. The patch prevents this and > other similar tools (e.g. KMSAN) from flagging this behavior in the future. Instead of changing all the internal users wouldn't it be better to add this check once in netlink_unicast_kernel?