Re: svn commit: r346319 - head/sys/netpfil/pf
Kristof,
On Wed, Apr 17, 2019 at 04:42:54PM +, Kristof Provost wrote:
K> Modified: head/sys/netpfil/pf/pf_ioctl.c
K>
==
K> --- head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:31:30 2019
(r346318)
K> +++ head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:42:54 2019
(r346319)
K> @@ -3103,24 +3103,24 @@ DIOCCHANGEADDR_error:
K> break;
K> }
K>
K> -PF_RULES_WLOCK();
K> +PF_RULES_RLOCK();
K> n = pfr_table_count(>pfrio_table, io->pfrio_flags);
K> io->pfrio_size = min(io->pfrio_size, n);
K> +PF_RULES_RUNLOCK();
K>
K> totlen = io->pfrio_size * sizeof(struct pfr_table);
K> pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
K> M_TEMP, M_NOWAIT);
K> if (pfrts == NULL) {
K> error = ENOMEM;
K> -PF_RULES_WUNLOCK();
K> break;
K> }
K> error = copyin(io->pfrio_buffer, pfrts, totlen);
K> if (error) {
K> free(pfrts, M_TEMP);
K> -PF_RULES_WUNLOCK();
K> break;
K> }
K> +PF_RULES_WLOCK();
K> error = pfr_set_tflags(pfrts, io->pfrio_size,
K> io->pfrio_setflag, io->pfrio_clrflag, >pfrio_nchange,
K> >pfrio_ndel, io->pfrio_flags | PFR_FLAG_USERIOCTL);
Couple comments:
1) Now we can malloc with M_WAITOK.
2) Are we sure that table count won't change while we dropped the lock?
--
Gleb Smirnoff
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r346319 - head/sys/netpfil/pf
On 17 Apr 2019, at 22:17, Gleb Smirnoff wrote:
Kristof,
On Wed, Apr 17, 2019 at 04:42:54PM +, Kristof Provost wrote:
K> Modified: head/sys/netpfil/pf/pf_ioctl.c
K>
==
K> --- head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:31:30
2019 (r346318)
K> +++ head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:42:54
2019 (r346319)
K> @@ -3103,24 +3103,24 @@ DIOCCHANGEADDR_error:
K> break;
K> }
K>
K> - PF_RULES_WLOCK();
K> + PF_RULES_RLOCK();
K> n = pfr_table_count(>pfrio_table, io->pfrio_flags);
K> io->pfrio_size = min(io->pfrio_size, n);
K> + PF_RULES_RUNLOCK();
K>
K> totlen = io->pfrio_size * sizeof(struct pfr_table);
K> pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
K> M_TEMP, M_NOWAIT);
K> if (pfrts == NULL) {
K> error = ENOMEM;
K> - PF_RULES_WUNLOCK();
K> break;
K> }
K> error = copyin(io->pfrio_buffer, pfrts, totlen);
K> if (error) {
K> free(pfrts, M_TEMP);
K> - PF_RULES_WUNLOCK();
K> break;
K> }
K> + PF_RULES_WLOCK();
K> error = pfr_set_tflags(pfrts, io->pfrio_size,
K> io->pfrio_setflag, io->pfrio_clrflag, >pfrio_nchange,
K> >pfrio_ndel, io->pfrio_flags | PFR_FLAG_USERIOCTL);
Couple comments:
1) Now we can malloc with M_WAITOK.
That’s a good point. I’ll see about changing that tomorrow.
2) Are we sure that table count won't change while we dropped the
lock?
No, the table count can indeed change while we’re unlocked. It
doesn’t really matter though. The initial count only serves to limit
the memory allocation to something sane. pfr_set_tflags() still does
appropriate checks.
It’s always been possible for the table count to change between user
space preparing its request and it being handled in the kernel, so that
was always a possibility.
Regards,
Kristof
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r346319 - head/sys/netpfil/pf
Author: kp
Date: Wed Apr 17 16:42:54 2019
New Revision: 346319
URL: https://svnweb.freebsd.org/changeset/base/346319
Log:
pf: Fix panic on invalid DIOCRSETTFLAGS
If during DIOCRSETTFLAGS pfrio_buffer is NULL copyin() will fault, which we're
not allowed to do with a lock held.
We must count the number of entries in the table and release the lock during
copyin(). Only then can we re-acquire the lock. Note that this is safe,
because
pfr_set_tflags() will check if the table and entries exist.
This was discovered by a local syzcaller instance.
MFC after:1 week
Event:Aberdeen hackathon 2019
Modified:
head/sys/netpfil/pf/pf_ioctl.c
Modified: head/sys/netpfil/pf/pf_ioctl.c
==
--- head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:31:30 2019
(r346318)
+++ head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:42:54 2019
(r346319)
@@ -3103,24 +3103,24 @@ DIOCCHANGEADDR_error:
break;
}
- PF_RULES_WLOCK();
+ PF_RULES_RLOCK();
n = pfr_table_count(>pfrio_table, io->pfrio_flags);
io->pfrio_size = min(io->pfrio_size, n);
+ PF_RULES_RUNLOCK();
totlen = io->pfrio_size * sizeof(struct pfr_table);
pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
M_TEMP, M_NOWAIT);
if (pfrts == NULL) {
error = ENOMEM;
- PF_RULES_WUNLOCK();
break;
}
error = copyin(io->pfrio_buffer, pfrts, totlen);
if (error) {
free(pfrts, M_TEMP);
- PF_RULES_WUNLOCK();
break;
}
+ PF_RULES_WLOCK();
error = pfr_set_tflags(pfrts, io->pfrio_size,
io->pfrio_setflag, io->pfrio_clrflag, >pfrio_nchange,
>pfrio_ndel, io->pfrio_flags | PFR_FLAG_USERIOCTL);
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r346319 - head/sys/netpfil/pf
On 17 Apr 2019, at 22:17, Gleb Smirnoff wrote:
Kristof,
On Wed, Apr 17, 2019 at 04:42:54PM +, Kristof Provost wrote:
K> Modified: head/sys/netpfil/pf/pf_ioctl.c
K>
==
K> --- head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:31:30
2019 (r346318)
K> +++ head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:42:54
2019 (r346319)
K> @@ -3103,24 +3103,24 @@ DIOCCHANGEADDR_error:
K> break;
K> }
K>
K> - PF_RULES_WLOCK();
K> + PF_RULES_RLOCK();
K> n = pfr_table_count(>pfrio_table, io->pfrio_flags);
K> io->pfrio_size = min(io->pfrio_size, n);
K> + PF_RULES_RUNLOCK();
K>
K> totlen = io->pfrio_size * sizeof(struct pfr_table);
K> pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
K> M_TEMP, M_NOWAIT);
K> if (pfrts == NULL) {
K> error = ENOMEM;
K> - PF_RULES_WUNLOCK();
K> break;
K> }
K> error = copyin(io->pfrio_buffer, pfrts, totlen);
K> if (error) {
K> free(pfrts, M_TEMP);
K> - PF_RULES_WUNLOCK();
K> break;
K> }
K> + PF_RULES_WLOCK();
K> error = pfr_set_tflags(pfrts, io->pfrio_size,
K> io->pfrio_setflag, io->pfrio_clrflag, >pfrio_nchange,
K> >pfrio_ndel, io->pfrio_flags | PFR_FLAG_USERIOCTL);
Couple comments:
1) Now we can malloc with M_WAITOK.
That’s a good point. I’ll see about changing that tomorrow.
2) Are we sure that table count won't change while we dropped the
lock?
No, the table count can indeed change while we’re unlocked. It
doesn’t really matter though. The initial count only serves to limit
the memory allocation to something sane. pfr_set_tflags() still does
appropriate checks.
It’s always been possible for the table count to change between user
space preparing its request and it being handled in the kernel, so that
was always a possibility.
Regards,
Kristof
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r346319 - head/sys/netpfil/pf
Kristof,
On Wed, Apr 17, 2019 at 04:42:54PM +, Kristof Provost wrote:
K> Modified: head/sys/netpfil/pf/pf_ioctl.c
K>
==
K> --- head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:31:30 2019
(r346318)
K> +++ head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:42:54 2019
(r346319)
K> @@ -3103,24 +3103,24 @@ DIOCCHANGEADDR_error:
K> break;
K> }
K>
K> -PF_RULES_WLOCK();
K> +PF_RULES_RLOCK();
K> n = pfr_table_count(>pfrio_table, io->pfrio_flags);
K> io->pfrio_size = min(io->pfrio_size, n);
K> +PF_RULES_RUNLOCK();
K>
K> totlen = io->pfrio_size * sizeof(struct pfr_table);
K> pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
K> M_TEMP, M_NOWAIT);
K> if (pfrts == NULL) {
K> error = ENOMEM;
K> -PF_RULES_WUNLOCK();
K> break;
K> }
K> error = copyin(io->pfrio_buffer, pfrts, totlen);
K> if (error) {
K> free(pfrts, M_TEMP);
K> -PF_RULES_WUNLOCK();
K> break;
K> }
K> +PF_RULES_WLOCK();
K> error = pfr_set_tflags(pfrts, io->pfrio_size,
K> io->pfrio_setflag, io->pfrio_clrflag, >pfrio_nchange,
K> >pfrio_ndel, io->pfrio_flags | PFR_FLAG_USERIOCTL);
Couple comments:
1) Now we can malloc with M_WAITOK.
2) Are we sure that table count won't change while we dropped the lock?
--
Gleb Smirnoff
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r346319 - head/sys/netpfil/pf
Author: kp
Date: Wed Apr 17 16:42:54 2019
New Revision: 346319
URL: https://svnweb.freebsd.org/changeset/base/346319
Log:
pf: Fix panic on invalid DIOCRSETTFLAGS
If during DIOCRSETTFLAGS pfrio_buffer is NULL copyin() will fault, which we're
not allowed to do with a lock held.
We must count the number of entries in the table and release the lock during
copyin(). Only then can we re-acquire the lock. Note that this is safe,
because
pfr_set_tflags() will check if the table and entries exist.
This was discovered by a local syzcaller instance.
MFC after:1 week
Event:Aberdeen hackathon 2019
Modified:
head/sys/netpfil/pf/pf_ioctl.c
Modified: head/sys/netpfil/pf/pf_ioctl.c
==
--- head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:31:30 2019
(r346318)
+++ head/sys/netpfil/pf/pf_ioctl.c Wed Apr 17 16:42:54 2019
(r346319)
@@ -3103,24 +3103,24 @@ DIOCCHANGEADDR_error:
break;
}
- PF_RULES_WLOCK();
+ PF_RULES_RLOCK();
n = pfr_table_count(>pfrio_table, io->pfrio_flags);
io->pfrio_size = min(io->pfrio_size, n);
+ PF_RULES_RUNLOCK();
totlen = io->pfrio_size * sizeof(struct pfr_table);
pfrts = mallocarray(io->pfrio_size, sizeof(struct pfr_table),
M_TEMP, M_NOWAIT);
if (pfrts == NULL) {
error = ENOMEM;
- PF_RULES_WUNLOCK();
break;
}
error = copyin(io->pfrio_buffer, pfrts, totlen);
if (error) {
free(pfrts, M_TEMP);
- PF_RULES_WUNLOCK();
break;
}
+ PF_RULES_WLOCK();
error = pfr_set_tflags(pfrts, io->pfrio_size,
io->pfrio_setflag, io->pfrio_clrflag, >pfrio_nchange,
>pfrio_ndel, io->pfrio_flags | PFR_FLAG_USERIOCTL);
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
