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

[Eric Dumazet <eric.dumazet@gmail.com>]

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;
}