net/packet: initialize val in packet_getsockopt()
Subsystems (3)
PATCH v2 1/1 net/packet: initialize val in packet_getsockopt()
In the case getsockopt() is called with PACKET_HDRLEN and optlen < 4 |val| remains uninitialized and the syscall may behave differently depending on its value. This doesn't have security consequences (as the uninit bytes aren't copied back), but it's still cleaner to initialize |val| and ensure optlen is not less than sizeof(int).
This bug has been detected with KMSAN.
---
v2: - if len < sizeof(int), make it 0
KMSAN report below:
==================================================================
BUG: KMSAN: use of unitialized memory in packet_getsockopt+0xb9b/0xbe0 inter: 0 CPU: 0 PID: 1036 Comm: probe Tainted: G B 4.11.0-rc5+ #2444 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:16 dump_stack+0x143/0x1b0 lib/dump_stack.c:52 kmsan_report+0x16b/0x1e0 mm/kmsan/kmsan.c:1078 __kmsan_warning_32+0x5c/0xa0 mm/kmsan/kmsan_instr.c:510 packet_getsockopt+0xb9b/0xbe0 net/packet/af_packet.c:3839 SYSC_getsockopt+0x495/0x540 net/socket.c:1829 SyS_getsockopt+0xb0/0xd0 net/socket.c:1811 entry_SYSCALL_64_fastpath+0x13/0x94 arch/x86/entry/entry_64.S:204 RIP: 0033:0x436d8a RSP: 002b:00007ffce54e52c8 EFLAGS: 00000203 ORIG_RAX: 0000000000000037 RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000436d8a RDX: 000000000000000b RSI: 0000000000000107 RDI: 0000000000000003 RBP: 00007ffce54e52b0 R08: 00007ffce54e52d8 R09: 0000000000000004 R10: 00007ffce54e52d4 R11: 0000000000000203 R12: 00007ffce54e53c8 R13: 00007ffce54e53d8 R14: 0000000000000002 R15: 0000000000000000 origin description: ----val@packet_getsockopt (origin=00000000f6600052) local variable created at: packet_getsockopt+0xcd/0xbe0 net/packet/af_packet.c:3789 SYSC_getsockopt+0x495/0x540 net/socket.c:1829 ================================================================== --- net/packet/af_packet.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
net/packet/af_packet.c +3 -1
@@ -3787,7 +3787,7 @@ static int packet_getsockopt(struct socket *sock, int level, int optname, char __user *optval, int __user *optlen) { int len; -int val, lv =sizeof (val); +int val =0 , lv =sizeof (val);struct sock *sk = sock->sk;struct packet_sock *po =pkt_sk (sk);void *data = &val;
@@ -3836,6 +3836,8 @@ static int packet_getsockopt(struct socket *sock, int level, int optname,case PACKET_HDRLEN :if (len >sizeof (int )) len =sizeof (int ); +if (len <sizeof (int )) + len =0 ;if (copy_from_user (&val, optval, len))return -EFAULT ;switch (val) {
Signature · 1 line
2.12.2.816.g2cccc81164-goog
David Miller
maintainerFrom: Alexander Potapenko <[email protected]>
Date: Mon, 24 Apr 2017 14:59:14 +0200
Quoted 11 lines
In the case
getsockopt()is called withPACKET_HDRLENand optlen < 4 |val| remains uninitialized and the syscall may behave differently depending on its value. This doesn't have security consequences (as the uninit bytes aren't copied back), but it's still cleaner to initialize |val| and ensure optlen is not less than sizeof(int).This bug has been detected with KMSAN.
Signed-off-by Alexander Potapenko <[email protected]>---
v2: - if len < sizeof(int), make it 0
No, you should signal an error if the len is too small.
Returning zero bytes to userspace silently makes the user think that he got the data he asked for.
Quoted 9 lines · David Miller
From: Alexander Potapenko <[email protected]>
Date: Mon, 24 Apr 2017 14:59:14 +0200Quoted 6 lines
In the case
getsockopt()is called withPACKET_HDRLENand optlen < 4 |val| remains uninitialized and the syscall may behave differently depending on its value. This doesn't have security consequences (as the uninit bytes aren't copied back), but it's still cleaner to initialize |val| and ensure optlen is not less than sizeof(int).
Quoted 7 lines
Quoted 5 lines
This bug has been detected with KMSAN.
Signed-off-by Alexander Potapenko <[email protected]>---
v2: - if len < sizeof(int), make it 0No, you should signal an error if the len is too small.
According to manpages, only setsockopt() may return EINVAL. Is it ok to change the behavior of getsockopt() to return EINVAL in this case? (I.e. won't we break existing users that don't expect it?)
Quoted 2 lines
Returning zero bytes to userspace silently makes the user think that he got the data he asked for.
Signature · 10 lines
Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
David Miller
maintainerFrom: Alexander Potapenko <[email protected]>
Date: Tue, 25 Apr 2017 18:27:04 +0200
Quoted 12 lines · David Miller
Quoted 11 lines
From: Alexander Potapenko <[email protected]>
Date: Mon, 24 Apr 2017 14:59:14 +0200Quoted 8 lines
In the case
getsockopt()is called withPACKET_HDRLENand optlen < 4 |val| remains uninitialized and the syscall may behave differently depending on its value. This doesn't have security consequences (as the uninit bytes aren't copied back), but it's still cleaner to initialize |val| and ensure optlen is not less than sizeof(int).This bug has been detected with KMSAN.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Signed-off-by Alexander Potapenko <[email protected]>---
v2: - if len < sizeof(int), make it 0No, you should signal an error if the len is too small.
According to manpages, only
setsockopt()may return EINVAL. Is it ok to change the behavior ofgetsockopt()to return EINVAL in this case? (I.e. won't we break existing users that don't expect it?)
They are currently getting corrupt data depending upon the endianness, so -EINVAL is a serious improvement.
Quoted 19 lines · David Miller
From: Alexander Potapenko <[email protected]>
Date: Tue, 25 Apr 2017 18:27:04 +0200Quoted 16 lines · David Miller
Quoted 15 lines
From: Alexander Potapenko <[email protected]>
Date: Mon, 24 Apr 2017 14:59:14 +0200Quoted 11 lines
In the case
getsockopt()is called withPACKET_HDRLENand optlen < 4 |val| remains uninitialized and the syscall may behave differently depending on its value. This doesn't have security consequences (as the uninit bytes aren't copied back), but it's still cleaner to initialize |val| and ensure optlen is not less than sizeof(int).This bug has been detected with KMSAN.
Signed-off-by Alexander Potapenko <[email protected]>---
v2: - if len < sizeof(int), make it 0
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
No, you should signal an error if the len is too small.
According to manpages, only
setsockopt()may return EINVAL. Is it ok to change the behavior ofgetsockopt()to return EINVAL in this case? (I.e. won't we break existing users that don't expect it?)They are currently getting corrupt data depending upon the endianness, so
-EINVALis a serious improvement.
On a second glance getsockopt() already returns -EINVAL in some cases, so man is already imprecise.
Signature · 10 lines
Alexander Potapenko Software Engineer Google Germany GmbH Erika-Mann-Straße, 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg