Skip to content

Commit

Permalink
pf: don't panic on missing mtag
Browse files Browse the repository at this point in the history 
Somewhat similar to the previous fix, but specifically tailored
for ALTQ and not generally hogging system resources...
  • Loading branch information
@fichtner
fichtner committed May 8, 2015
1 parent b717d68 commit 6cec108
Showing 1 changed file with 24 additions and 13 deletions.
37 changes: 24 additions & 13 deletions sys/netpfil/pf/pf.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5982,13 +5982,18 @@ pf_test(int dir, struct ifnet *ifp, struct mbuf **m0, struct inpcb *inp)
pd.act.qid = r->qid;
}
if (action == PF_PASS && pd.act.qid) {
if (pqid || (pd.tos & IPTOS_LOWDELAY))
pd.pf_mtag->qid = pd.act.pqid;
else
pd.pf_mtag->qid = pd.act.qid;
/* add hints for ecn */
pd.pf_mtag->hdr = h;

if (pd.pf_mtag == NULL &&
((pd.pf_mtag = pf_get_mtag(m)) == NULL)) {
action = PF_DROP;

This comment has been minimized.

Sign in to view

Copy link
@opntr

opntr May 8, 2015

Seems good, thanks!
I'm not familiar with pf's code, but the REASON_SET(&reason, PFRES_MEMORY) was not missing from this patch here?
} else {
if (pqid || (pd.tos & IPTOS_LOWDELAY)) {
pd.pf_mtag->qid = pd.act.pqid;
} else {
pd.pf_mtag->qid = pd.act.qid;
}
/* add hints for ecn */
pd.pf_mtag->hdr = h;
}
}
#endif /* ALTQ */

Expand Down Expand Up @@ -6426,12 +6431,18 @@ pf_test6(int dir, struct ifnet *ifp, struct mbuf **m0, struct inpcb *inp)
pd.act.qid = r->qid;
}
if (action == PF_PASS && pd.act.qid) {
if (pd.tos & IPTOS_LOWDELAY)
pd.pf_mtag->qid = pd.act.pqid;
else
pd.pf_mtag->qid = pd.act.qid;
/* add hints for ecn */
pd.pf_mtag->hdr = h;
if (pd.pf_mtag == NULL &&
((pd.pf_mtag = pf_get_mtag(m)) == NULL)) {
action = PF_DROP;

This comment has been minimized.

Sign in to view

Copy link
@opntr

opntr May 8, 2015

And here?
} else {
if (pd.tos & IPTOS_LOWDELAY) {
pd.pf_mtag->qid = pd.act.pqid;
} else {
pd.pf_mtag->qid = pd.act.qid;
}
/* add hints for ecn */
pd.pf_mtag->hdr = h;
}
}
#endif /* ALTQ */

Expand Down

11 comments on commit 6cec108

@fichtner
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yo @lattera @opntr this may be relevant to your interest: 10-STABLE and probably 11-CURRENT have a similar fix that will panic when pf_get_mtag() fails. Got MFC'd from 11-CURRENT some time after 10.1.

@fichtner
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/freebsd/freebsd/blob/stable/10/sys/netpfil/pf/pf.c#L6295-6299 sets drop, but dereferences unconditionally below... pew pew pew

@fichtner
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@opntr I didn't look for the STABLE patch until I patched this so I don't have REASON_SET() in there. A mix of both patches (really only the the else condition) should be added to STABLE. I can patch this if on top of HardenedBSD 10-STABLE if you want?

@opntr
Copy link

@opntr opntr commented on 6cec108 May 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly fine what the current the current status is, but the REASON_SET() is not mandatory? What affect when this set to foo or not set to foo?

@opntr
Copy link

@opntr opntr commented on 6cec108 May 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I've added you to our contributors group, this means you have access to our playground repo.

@opntr
Copy link

@opntr opntr commented on 6cec108 May 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, do you notified the FreeBSD about this issue?

@fichtner
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@opntr nope, FreeBSD isn't in the loop yet. I've cooked up a patch for all the spots that have this sloppy behaviour: HardenedBSD/hardenedBSD-playground@6c36a04 (for 10-STABLE)

how do we get this upstream?

@opntr
Copy link

@opntr opntr commented on 6cec108 May 15, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=200222

@opntr
Copy link

@opntr opntr commented on 6cec108 May 15, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please create a FreeBSD bugzilla account? I like to add you to CC list.

@fichtner
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc'ed myself, thanks!

@opntr
Copy link

@opntr opntr commented on 6cec108 May 15, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HardenedBSD/hardenedBSD@246b23f
HardenedBSD/hardenedBSD@d89fd6e

Please sign in to comment.