Re: CVS commit: src/sys/dev

[Martin Husemann <martin%NetBSD.org@localhost>]

On Sat, Dec 26, 2020 at 02:50:50PM +0000, Nia Alarie wrote:
> Module Name:	src
> Committed By:	nia
> Date:		Sat Dec 26 14:50:50 UTC 2020
> 
> Modified Files:
> 	src/sys/dev: fss.c
> 
> Log Message:
> Check the return value of device_lookup_private against NULL.
> 
> Reported-by: syzbot+06561ba90b6e618ce6d0%syzkaller.appspotmail.com@localhost
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.109 -r1.110 src/sys/dev/fss.c

I don't think this fix is correct.

The reproducer creates a device node:

	  c---------  1 root  wheel  163, 524431 Jan 15 12:19 file0

which on amd64 would be /dev/rfss524431
and opens it, then exists.

All this in a loop with 6 concurrent processes. Since all mknod operations
fail once the node exists, it boils down to opening the rfss device
(which creates the softc and stores it in the driver cd state) and then
exiting (which closes the device node, as last close leading to freeing
of the softc and putting a NULL pointer into the cd state).

The softc never can be NULL when ariving at fss_close for an existing open
device. If this happens, something else must have gone terribly wrong
and we need to investigate closer.

Unfortunately I have been unable to reproduce the issue with the syzbot
generated reproducer (and aware that syzbot uses KASAN, but I sprinkled
tests for sc == NULL and panic calls instead).

Martin

/*
 * generates a char dev:
 *		c---------  1 root  wheel  163, 524431 Jan 15 12:19 file0
 *
 *   which would be: /dev/rfss524431
 *
 * CALL  compat_50_mknod(0x20000080,0x2000,0x8000a38f)
 * NAMI  "./file0"
 */

// https://syzkaller.appspot.com/bug?id=cb6a2dba2e6f2ea5c3fb3f3e4a27fd225bba1e41
// autogenerated by syzkaller (https://github.com/google/syzkaller)

#define _GNU_SOURCE

#include <endian.h>
#include <pwd.h>
#include <signal.h>
#include <stdarg.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <time.h>
#include <unistd.h>

static unsigned long long procid;

static void kill_and_wait(int pid, int* status)
{
  kill(pid, SIGKILL);
  while (waitpid(-1, status, 0) != pid) {
  }
}

static void sleep_ms(uint64_t ms)
{
  usleep(ms * 1000);
}

static uint64_t current_time_ms(void)
{
  struct timespec ts;
  if (clock_gettime(CLOCK_MONOTONIC, &ts))
    exit(1);
  return (uint64_t)ts.tv_sec * 1000 + (uint64_t)ts.tv_nsec / 1000000;
}

static void execute_one(void);

#define WAIT_FLAGS 0

static void loop(void)
{
  int iter = 0;
  for (;; iter++) {
    int pid = fork();
    if (pid < 0)
      exit(1);
    if (pid == 0) {
      execute_one();
      exit(0);
    }
    int status = 0;
    uint64_t start = current_time_ms();
    for (;;) {
      if (waitpid(-1, &status, WNOHANG | WAIT_FLAGS) == pid)
        break;
      sleep_ms(1);
      if (current_time_ms() - start < 5 * 1000)
        continue;
      kill_and_wait(pid, &status);
      break;
    }
  }
}

#ifndef SYS_compat_50_mknod
#define SYS_compat_50_mknod 14
#endif
#ifndef SYS_mmap
#define SYS_mmap 197
#endif
#ifndef SYS_open
#define SYS_open 5
#endif

void execute_one(void)
{
  memcpy((void*)0x20000080, "./file0\000", 8);
  syscall(SYS_compat_50_mknod, 0x20000080ul, 0x2000ul, 0x8000a38f);
  memcpy((void*)0x20000180, "./file0\000", 8);
  syscall(SYS_open, 0x20000180ul, 0ul, 0ul);
}
int main(void)
{
  syscall(SYS_mmap, 0x20000000ul, 0x1000000ul, 3ul, 0x1012ul, -1, 0ul, 0ul);
  for (procid = 0; procid < 6; procid++) {
    if (fork() == 0) {
      loop();
    }
  }
  sleep(1000000);
  return 0;
}