git: 7a808c5ee329 - main - pf: Improve pf_rule input validation

[Kristof Provost]

The branch main has been updated by kp:

URL: https://cgit.FreeBSD.org/src/commit/?id=7a808c5ee3296fdb72d8e8bc6c7ad6f316a520ab

commit 7a808c5ee3296fdb72d8e8bc6c7ad6f316a520ab
Author:     Kristof Provost <kp at FreeBSD.org>
AuthorDate: 2021-01-26 07:56:51 +0000
Commit:     Kristof Provost <kp at FreeBSD.org>
CommitDate: 2021-01-27 15:42:14 +0000

    pf: Improve pf_rule input validation
    
    Move the validation checks to pf_rule_to_krule() to reduce duplication.
    This also makes the checks consistent across different ioctls.
    
    Reported-by:    syzbot+e9632d7ad17398f0bd8f at syzkaller.appspotmail.com
    Reviewed by:    tuexen@, donner@
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D28362
---
 sys/netpfil/pf/pf_ioctl.c | 72 +++++++++++++++++++++++++++--------------------
 1 file changed, 41 insertions(+), 31 deletions(-)

diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c
index 60c38a980d1e..644a091808cd 100644
--- a/sys/netpfil/pf/pf_ioctl.c
+++ b/sys/netpfil/pf/pf_ioctl.c
@@ -1557,10 +1557,39 @@ pf_krule_to_rule(const struct pf_krule *krule, struct pf_rule *rule)
 	rule->u_src_nodes = counter_u64_fetch(krule->src_nodes);
 }
 
-static void
+static int
 pf_rule_to_krule(const struct pf_rule *rule, struct pf_krule *krule)
 {
 
+#ifndef INET
+	if (rule->af == AF_INET) {
+		return (EAFNOSUPPORT);
+	}
+#endif /* INET */
+#ifndef INET6
+	if (rule->af == AF_INET6) {
+		return (EAFNOSUPPORT);
+	}
+#endif /* INET6 */
+
+	if (rule->src.addr.type != PF_ADDR_ADDRMASK &&
+	    rule->src.addr.type != PF_ADDR_DYNIFTL &&
+	    rule->src.addr.type != PF_ADDR_TABLE) {
+		return (EINVAL);
+	}
+	if (rule->src.addr.p.dyn != NULL) {
+		return (EINVAL);
+	}
+
+	if (rule->dst.addr.type != PF_ADDR_ADDRMASK &&
+	    rule->dst.addr.type != PF_ADDR_DYNIFTL &&
+	    rule->dst.addr.type != PF_ADDR_TABLE) {
+		return (EINVAL);
+	}
+	if (rule->dst.addr.p.dyn != NULL) {
+		return (EINVAL);
+	}
+
 	bzero(krule, sizeof(*krule));
 
 	bcopy(&rule->src, &krule->src, sizeof(rule->src));
@@ -1641,6 +1670,8 @@ pf_rule_to_krule(const struct pf_rule *rule, struct pf_krule *krule)
 	krule->set_prio[1] = rule->set_prio[1];
 
 	bcopy(&rule->divert, &krule->divert, sizeof(krule->divert));
+
+	return (0);
 }
 
 static int
@@ -1815,26 +1846,13 @@ pfioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flags, struct thread *td
 			error = EINVAL;
 			break;
 		}
-		if (pr->rule.src.addr.p.dyn != NULL ||
-		    pr->rule.dst.addr.p.dyn != NULL) {
-			error = EINVAL;
-			break;
-		}
-#ifndef INET
-		if (pr->rule.af == AF_INET) {
-			error = EAFNOSUPPORT;
-			break;
-		}
-#endif /* INET */
-#ifndef INET6
-		if (pr->rule.af == AF_INET6) {
-			error = EAFNOSUPPORT;
-			break;
-		}
-#endif /* INET6 */
 
 		rule = malloc(sizeof(*rule), M_PFRULE, M_WAITOK);
-		pf_rule_to_krule(&pr->rule, rule);
+		error = pf_rule_to_krule(&pr->rule, rule);
+		if (error != 0) {
+			free(rule, M_PFRULE);
+			break;
+		}
 
 		if (rule->ifname[0])
 			kif = pf_kkif_create(M_WAITOK);
@@ -2090,20 +2108,12 @@ DIOCADDRULE_error:
 		}
 
 		if (pcr->action != PF_CHANGE_REMOVE) {
-#ifndef INET
-			if (pcr->rule.af == AF_INET) {
-				error = EAFNOSUPPORT;
-				break;
-			}
-#endif /* INET */
-#ifndef INET6
-			if (pcr->rule.af == AF_INET6) {
-				error = EAFNOSUPPORT;
+			newrule = malloc(sizeof(*newrule), M_PFRULE, M_WAITOK);
+			error = pf_rule_to_krule(&pcr->rule, newrule);
+			if (error != 0) {
+				free(newrule, M_PFRULE);
 				break;
 			}
-#endif /* INET6 */
-			newrule = malloc(sizeof(*newrule), M_PFRULE, M_WAITOK);
-			pf_rule_to_krule(&pcr->rule, newrule);
 
 			if (newrule->ifname[0])
 				kif = pf_kkif_create(M_WAITOK);