diff mbox

net: don't call strlen() on the user buffer in packet_bind_spkt()

Message ID 20170228131759.110380-1-glider@google.com
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Alexander Potapenko Feb. 28, 2017, 1:17 p.m. UTC 
KMSAN (KernelMemorySanitizer, a new error detection tool) reports use of
uninitialized memory in packet_bind_spkt():

Comments

Eric Dumazet Feb. 28, 2017, 1:33 p.m. UTC | #1 
On Tue, 2017-02-28 at 14:17 +0100, Alexander Potapenko wrote:
> KMSAN (KernelMemorySanitizer, a new error detection tool) reports use of
> uninitialized memory in packet_bind_spkt():
> 
> ==================================================================
> BUG: KMSAN: use of unitialized memory
> CPU: 0 PID: 1074 Comm: packet Not tainted 4.8.0-rc6+ #1891
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
> 01/01/2011
>  0000000000000000 ffff88006b6dfc08 ffffffff82559ae8 ffff88006b6dfb48
>  ffffffff818a7c91 ffffffff85b9c870 0000000000000092 ffffffff85b9c550
>  0000000000000000 0000000000000092 00000000ec400911 0000000000000002
> Call Trace:
>  [<     inline     >] __dump_stack lib/dump_stack.c:15
>  [<ffffffff82559ae8>] dump_stack+0x238/0x290 lib/dump_stack.c:51
>  [<ffffffff818a6626>] kmsan_report+0x276/0x2e0 mm/kmsan/kmsan.c:1003
>  [<ffffffff818a783b>] __msan_warning+0x5b/0xb0
> mm/kmsan/kmsan_instr.c:424
>  [<     inline     >] strlen lib/string.c:484
>  [<ffffffff8259b58d>] strlcpy+0x9d/0x200 lib/string.c:144
>  [<ffffffff84b2eca4>] packet_bind_spkt+0x144/0x230
> net/packet/af_packet.c:3132
>  [<ffffffff84242e4d>] SYSC_bind+0x40d/0x5f0 net/socket.c:1370
>  [<ffffffff84242a22>] SyS_bind+0x82/0xa0 net/socket.c:1356
>  [<ffffffff8515991b>] entry_SYSCALL_64_fastpath+0x13/0x8f
> arch/x86/entry/entry_64.o:?
> chained origin: 00000000eba00911
>  [<ffffffff810bb787>] save_stack_trace+0x27/0x50
> arch/x86/kernel/stacktrace.c:67
>  [<     inline     >] kmsan_save_stack_with_flags mm/kmsan/kmsan.c:322
>  [<     inline     >] kmsan_save_stack mm/kmsan/kmsan.c:334
>  [<ffffffff818a59f8>] kmsan_internal_chain_origin+0x118/0x1e0
> mm/kmsan/kmsan.c:527
>  [<ffffffff818a7773>] __msan_set_alloca_origin4+0xc3/0x130
> mm/kmsan/kmsan_instr.c:380
>  [<ffffffff84242b69>] SYSC_bind+0x129/0x5f0 net/socket.c:1356
>  [<ffffffff84242a22>] SyS_bind+0x82/0xa0 net/socket.c:1356
>  [<ffffffff8515991b>] entry_SYSCALL_64_fastpath+0x13/0x8f
> arch/x86/entry/entry_64.o:?
> origin description: ----address@SYSC_bind (origin=00000000eb400911)
> ==================================================================
> (the line numbers are relative to 4.8-rc6, but the bug persists
> upstream)
> 
> , when I run the following program as root:
> 
> =====================================
>  #include <string.h>
>  #include <sys/socket.h>
>  #include <netpacket/packet.h>
>  #include <net/ethernet.h>
> 
>  int main() {
>    struct sockaddr addr;
>    memset(&addr, 0xff, sizeof(addr));
>    addr.sa_family = AF_PACKET;
>    int fd = socket(PF_PACKET, SOCK_PACKET, htons(ETH_P_ALL));
>    bind(fd, &addr, sizeof(addr));
>    return 0;
>  }
> =====================================
> 
> This happens because addr.sa_data copied from the userspace is not
> zero-terminated, and copying it with strlcpy() in packet_bind_spkt()
> results in calling strlen() on the kernel copy of that non-terminated
> buffer.
> 
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
>  net/packet/af_packet.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 2bd0d1949312..1e7992f3e0a8 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -3111,7 +3111,11 @@ static int packet_bind_spkt(struct socket *sock, struct sockaddr *uaddr,
>  
>  	if (addr_len != sizeof(struct sockaddr))
>  		return -EINVAL;
> -	strlcpy(name, uaddr->sa_data, sizeof(name));
> +	/* uaddr->sa_data comes from the userspace, it's not guaranteed to be
> +	 * zero-terminated.
> +	 */
> +	name[14] = '\0';
> +	strncpy(name, uaddr->sa_data, sizeof(name));
>  
>  	return packet_do_bind(sk, name, 0, pkt_sk(sk)->num);
>  }

It looks a bug in this implementation of strlcpy() then.

sizeof(name) is 15.

If you use strncpy(X, uaddr->sa_data, 15) , then you might access
uaddr->sa_data[14] and this would still be wrong, since sa_data has 14
bytes only :


struct sockaddr {
  sa_family_t sa_family;
  char        sa_data[14];
};


So I do not believe your patch is right.
Alexander Potapenko Feb. 28, 2017, 1:39 p.m. UTC | #2 
On Tue, Feb 28, 2017 at 2:33 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-02-28 at 14:17 +0100, Alexander Potapenko wrote:
>> KMSAN (KernelMemorySanitizer, a new error detection tool) reports use of
>> uninitialized memory in packet_bind_spkt():
>>
>> ==================================================================
>> BUG: KMSAN: use of unitialized memory
>> CPU: 0 PID: 1074 Comm: packet Not tainted 4.8.0-rc6+ #1891
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
>> 01/01/2011
>>  0000000000000000 ffff88006b6dfc08 ffffffff82559ae8 ffff88006b6dfb48
>>  ffffffff818a7c91 ffffffff85b9c870 0000000000000092 ffffffff85b9c550
>>  0000000000000000 0000000000000092 00000000ec400911 0000000000000002
>> Call Trace:
>>  [<     inline     >] __dump_stack lib/dump_stack.c:15
>>  [<ffffffff82559ae8>] dump_stack+0x238/0x290 lib/dump_stack.c:51
>>  [<ffffffff818a6626>] kmsan_report+0x276/0x2e0 mm/kmsan/kmsan.c:1003
>>  [<ffffffff818a783b>] __msan_warning+0x5b/0xb0
>> mm/kmsan/kmsan_instr.c:424
>>  [<     inline     >] strlen lib/string.c:484
>>  [<ffffffff8259b58d>] strlcpy+0x9d/0x200 lib/string.c:144
>>  [<ffffffff84b2eca4>] packet_bind_spkt+0x144/0x230
>> net/packet/af_packet.c:3132
>>  [<ffffffff84242e4d>] SYSC_bind+0x40d/0x5f0 net/socket.c:1370
>>  [<ffffffff84242a22>] SyS_bind+0x82/0xa0 net/socket.c:1356
>>  [<ffffffff8515991b>] entry_SYSCALL_64_fastpath+0x13/0x8f
>> arch/x86/entry/entry_64.o:?
>> chained origin: 00000000eba00911
>>  [<ffffffff810bb787>] save_stack_trace+0x27/0x50
>> arch/x86/kernel/stacktrace.c:67
>>  [<     inline     >] kmsan_save_stack_with_flags mm/kmsan/kmsan.c:322
>>  [<     inline     >] kmsan_save_stack mm/kmsan/kmsan.c:334
>>  [<ffffffff818a59f8>] kmsan_internal_chain_origin+0x118/0x1e0
>> mm/kmsan/kmsan.c:527
>>  [<ffffffff818a7773>] __msan_set_alloca_origin4+0xc3/0x130
>> mm/kmsan/kmsan_instr.c:380
>>  [<ffffffff84242b69>] SYSC_bind+0x129/0x5f0 net/socket.c:1356
>>  [<ffffffff84242a22>] SyS_bind+0x82/0xa0 net/socket.c:1356
>>  [<ffffffff8515991b>] entry_SYSCALL_64_fastpath+0x13/0x8f
>> arch/x86/entry/entry_64.o:?
>> origin description: ----address@SYSC_bind (origin=00000000eb400911)
>> ==================================================================
>> (the line numbers are relative to 4.8-rc6, but the bug persists
>> upstream)
>>
>> , when I run the following program as root:
>>
>> =====================================
>>  #include <string.h>
>>  #include <sys/socket.h>
>>  #include <netpacket/packet.h>
>>  #include <net/ethernet.h>
>>
>>  int main() {
>>    struct sockaddr addr;
>>    memset(&addr, 0xff, sizeof(addr));
>>    addr.sa_family = AF_PACKET;
>>    int fd = socket(PF_PACKET, SOCK_PACKET, htons(ETH_P_ALL));
>>    bind(fd, &addr, sizeof(addr));
>>    return 0;
>>  }
>> =====================================
>>
>> This happens because addr.sa_data copied from the userspace is not
>> zero-terminated, and copying it with strlcpy() in packet_bind_spkt()
>> results in calling strlen() on the kernel copy of that non-terminated
>> buffer.
>>
>> Signed-off-by: Alexander Potapenko <glider@google.com>
>> ---
>>  net/packet/af_packet.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index 2bd0d1949312..1e7992f3e0a8 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -3111,7 +3111,11 @@ static int packet_bind_spkt(struct socket *sock, struct sockaddr *uaddr,
>>
>>       if (addr_len != sizeof(struct sockaddr))
>>               return -EINVAL;
>> -     strlcpy(name, uaddr->sa_data, sizeof(name));
>> +     /* uaddr->sa_data comes from the userspace, it's not guaranteed to be
>> +      * zero-terminated.
>> +      */
>> +     name[14] = '\0';
>> +     strncpy(name, uaddr->sa_data, sizeof(name));
>>
>>       return packet_do_bind(sk, name, 0, pkt_sk(sk)->num);
>>  }
>
> It looks a bug in this implementation of strlcpy() then.
This depends on how we define the semantics of strlcpy().
The implementation in lib/string.c
(http://lxr.free-electrons.com/source/lib/string.c#L129) says that
we're copying a C-string |src|, which _may_ denote it should be
zero-terminated.
I would still call strnlen() instead of strlen() in strlcpy() though.
> sizeof(name) is 15.
>
> If you use strncpy(X, uaddr->sa_data, 15) , then you might access
> uaddr->sa_data[14] and this would still be wrong, since sa_data has 14
> bytes only :
>
>
> struct sockaddr {
>   sa_family_t sa_family;
>   char        sa_data[14];
> };
>
>
> So I do not believe your patch is right.
Sorry, I've sent a fixed patch while you were writing your comment.
Of course, we shouldn't copy more than 14 bytes.
>
diff mbox

Patch

==================================================================
BUG: KMSAN: use of unitialized memory
CPU: 0 PID: 1074 Comm: packet Not tainted 4.8.0-rc6+ #1891
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
01/01/2011
 0000000000000000 ffff88006b6dfc08 ffffffff82559ae8 ffff88006b6dfb48
 ffffffff818a7c91 ffffffff85b9c870 0000000000000092 ffffffff85b9c550
 0000000000000000 0000000000000092 00000000ec400911 0000000000000002
Call Trace:
 [<     inline     >] __dump_stack lib/dump_stack.c:15
 [<ffffffff82559ae8>] dump_stack+0x238/0x290 lib/dump_stack.c:51
 [<ffffffff818a6626>] kmsan_report+0x276/0x2e0 mm/kmsan/kmsan.c:1003
 [<ffffffff818a783b>] __msan_warning+0x5b/0xb0
mm/kmsan/kmsan_instr.c:424
 [<     inline     >] strlen lib/string.c:484
 [<ffffffff8259b58d>] strlcpy+0x9d/0x200 lib/string.c:144
 [<ffffffff84b2eca4>] packet_bind_spkt+0x144/0x230
net/packet/af_packet.c:3132
 [<ffffffff84242e4d>] SYSC_bind+0x40d/0x5f0 net/socket.c:1370
 [<ffffffff84242a22>] SyS_bind+0x82/0xa0 net/socket.c:1356
 [<ffffffff8515991b>] entry_SYSCALL_64_fastpath+0x13/0x8f
arch/x86/entry/entry_64.o:?
chained origin: 00000000eba00911
 [<ffffffff810bb787>] save_stack_trace+0x27/0x50
arch/x86/kernel/stacktrace.c:67
 [<     inline     >] kmsan_save_stack_with_flags mm/kmsan/kmsan.c:322
 [<     inline     >] kmsan_save_stack mm/kmsan/kmsan.c:334
 [<ffffffff818a59f8>] kmsan_internal_chain_origin+0x118/0x1e0
mm/kmsan/kmsan.c:527
 [<ffffffff818a7773>] __msan_set_alloca_origin4+0xc3/0x130
mm/kmsan/kmsan_instr.c:380
 [<ffffffff84242b69>] SYSC_bind+0x129/0x5f0 net/socket.c:1356
 [<ffffffff84242a22>] SyS_bind+0x82/0xa0 net/socket.c:1356
 [<ffffffff8515991b>] entry_SYSCALL_64_fastpath+0x13/0x8f
arch/x86/entry/entry_64.o:?
origin description: ----address@SYSC_bind (origin=00000000eb400911)
==================================================================
(the line numbers are relative to 4.8-rc6, but the bug persists
upstream)

, when I run the following program as root:

=====================================
 #include <string.h>
 #include <sys/socket.h>
 #include <netpacket/packet.h>
 #include <net/ethernet.h>

 int main() {
   struct sockaddr addr;
   memset(&addr, 0xff, sizeof(addr));
   addr.sa_family = AF_PACKET;
   int fd = socket(PF_PACKET, SOCK_PACKET, htons(ETH_P_ALL));
   bind(fd, &addr, sizeof(addr));
   return 0;
 }
=====================================

This happens because addr.sa_data copied from the userspace is not
zero-terminated, and copying it with strlcpy() in packet_bind_spkt()
results in calling strlen() on the kernel copy of that non-terminated
buffer.

Signed-off-by: Alexander Potapenko <glider@google.com>
---
 net/packet/af_packet.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2bd0d1949312..1e7992f3e0a8 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3111,7 +3111,11 @@  static int packet_bind_spkt(struct socket *sock, struct sockaddr *uaddr,
 
 	if (addr_len != sizeof(struct sockaddr))
 		return -EINVAL;
-	strlcpy(name, uaddr->sa_data, sizeof(name));
+	/* uaddr->sa_data comes from the userspace, it's not guaranteed to be
+	 * zero-terminated.
+	 */
+	name[14] = '\0';
+	strncpy(name, uaddr->sa_data, sizeof(name));
 
 	return packet_do_bind(sk, name, 0, pkt_sk(sk)->num);
 }