Hi tech,
for the other two DIOCX ioctls syzkaller showed that it is possible
to grab netlock while doing copyin.
The same problem should exist for DIOCXCOMMIT but syzkaller didn't
find it yet.
In case anybody can reproduce the witness lock order reversals the
syzkaller can produce, the diff below might address the problem.
mbuhl
Index: sys/net/pf_ioctl.c
===================================================================
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.383
diff -u -p -r1.383 pf_ioctl.c
--- sys/net/pf_ioctl.c 20 Jul 2022 09:33:11 -0000 1.383
+++ sys/net/pf_ioctl.c 20 Jul 2022 18:17:45 -0000
@@ -2621,13 +2621,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
}
ioe = malloc(sizeof(*ioe), M_TEMP, M_WAITOK);
table = malloc(sizeof(*table), M_TEMP, M_WAITOK);
- NET_LOCK();
- PF_LOCK();
/* first makes sure everything will succeed */
for (i = 0; i < io->size; i++) {
if (copyin(io->array+i, ioe, sizeof(*ioe))) {
- PF_UNLOCK();
- NET_UNLOCK();
free(table, M_TEMP, sizeof(*table));
free(ioe, M_TEMP, sizeof(*ioe));
error = EFAULT;
@@ -2635,13 +2631,13 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
}
if (strnlen(ioe->anchor, sizeof(ioe->anchor)) ==
sizeof(ioe->anchor)) {
- PF_UNLOCK();
- NET_UNLOCK();
free(table, M_TEMP, sizeof(*table));
free(ioe, M_TEMP, sizeof(*ioe));
error = ENAMETOOLONG;
goto fail;
}
+ NET_LOCK();
+ PF_LOCK();
switch (ioe->type) {
case PF_TRANS_TABLE:
rs = pf_find_ruleset(ioe->anchor);
@@ -2677,7 +2673,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
error = EINVAL;
goto fail;
}
+ PF_UNLOCK();
+ NET_UNLOCK();
}
+ NET_LOCK();
+ PF_LOCK();
/*
* Checked already in DIOCSETLIMIT, but check again as the
@@ -2696,9 +2696,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
}
/* now do the commit - no errors should happen here */
for (i = 0; i < io->size; i++) {
+ PF_UNLOCK();
+ NET_UNLOCK();
if (copyin(io->array+i, ioe, sizeof(*ioe))) {
- PF_UNLOCK();
- NET_UNLOCK();
free(table, M_TEMP, sizeof(*table));
free(ioe, M_TEMP, sizeof(*ioe));
error = EFAULT;
@@ -2706,13 +2706,13 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
}
if (strnlen(ioe->anchor, sizeof(ioe->anchor)) ==
sizeof(ioe->anchor)) {
- PF_UNLOCK();
- NET_UNLOCK();
free(table, M_TEMP, sizeof(*table));
free(ioe, M_TEMP, sizeof(*ioe));
error = ENAMETOOLONG;
goto fail;
}
+ NET_LOCK();
+ PF_LOCK();
switch (ioe->type) {
case PF_TRANS_TABLE:
memset(table, 0, sizeof(*table));
