netdev - Re: [PATCH 3/3] net: cleanly handle kernel vs user buffers for ->msg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b9728e02-e317-2aa6-9ed4-723ee3abfb78@gmail.com>
Date:   Wed, 13 May 2020 09:18:36 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Christoph Hellwig <hch@....de>,
        Eric Dumazet <eric.dumazet@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] net: cleanly handle kernel vs user buffers for
 ->msg_control



On 5/13/20 9:09 AM, Christoph Hellwig wrote:
> On Wed, May 13, 2020 at 08:41:57AM -0700, Eric Dumazet wrote:
>>> +	 * recv* side when msg_control_is_user is set, msg_control is the kernel
>>> +	 * buffer used for all other cases.
>>> +	 */
>>> +	union {
>>> +		void		*msg_control;
>>> +		void __user	*msg_control_user;
>>> +	};
>>> +	bool		msg_control_is_user : 1;
>>
>> Adding a field in this structure seems dangerous.
>>
>> Some users of 'struct msghdr '  define their own struct on the stack,
>> and are unaware of this new mandatory field.
>>
>> This bit contains garbage, crashes are likely to happen ?
>>
>> Look at IPV6_2292PKTOPTIONS for example.
> 
> I though of that, an that is why the field is structured as-is.  The idea
> is that the field only matters if:
> 
>  (1) we are in the recvmsg and friends path, and
>  (2) msg_control is non-zero
> 
> I went through the places that initialize msg_control to find any spot
> that would need an annotation.  The IPV6_2292PKTOPTIONS sockopt doesn't
> need one as it is using the msghdr in sendmsg-like context.
> 
> That being said while I did the audit I'd appreciate another look from
> people that know the networking code better than me of course.
> 

Please try the following syzbot repro, since it crashes after your patch.

// autogenerated by syzkaller (https://github.com/google/syzkaller)

#define _GNU_SOURCE

#include <endian.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <unistd.h>

uint64_t r[1] = {0xffffffffffffffff};

int main(void)
{
  syscall(__NR_mmap, 0x1ffff000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul);
  syscall(__NR_mmap, 0x20000000ul, 0x1000000ul, 7ul, 0x32ul, -1, 0ul);
  syscall(__NR_mmap, 0x21000000ul, 0x1000ul, 0ul, 0x32ul, -1, 0ul);
  intptr_t res = 0;

  // socket(AF_INET6, SOCK_STREAM, IPPROTO_IP) = 3
  res = syscall(__NR_socket, 0xaul, 1ul, 0);
  if (res != -1)
    r[0] = res;

  *(uint32_t*)0x20000080 = 7;
  // setsockopt(3, SOL_IPV6, IPV6_2292HOPLIMIT, [7], 4) = 0
  syscall(__NR_setsockopt, r[0], 0x29, 8, 0x20000080ul, 4ul);

  *(uint32_t*)0x20000040 = 0x18ff8;
  // getsockopt(3, SOL_IPV6, IPV6_2292PKTOPTIONS, "\24\0\0\0\0\0\0\0)\0\0\0\10\0\0\0\1\0\0\0\0\0\0\0", [102392->24]) = 0
  syscall(__NR_getsockopt, r[0], 0x29, 6, 0x20004040ul, 0x20000040ul);

  return 0;
}