On Thu, Jul 21, 2022 at 11:13:28AM +0200, Moritz Buhl wrote:
> 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
OK bluhm@
> 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));
