Page MenuHomeFreeBSD

pf: Improve input validation
ClosedPublic

Authored by kp on Apr 22 2020, 7:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 23, 11:51 AM
Unknown Object (File)
Thu, Mar 13, 8:04 PM
Unknown Object (File)
Feb 1 2025, 6:15 AM
Unknown Object (File)
Dec 27 2024, 3:18 AM
Unknown Object (File)
Dec 27 2024, 3:16 AM
Unknown Object (File)
Dec 15 2024, 10:31 AM
Unknown Object (File)
Dec 11 2024, 8:57 AM
Unknown Object (File)
Nov 30 2024, 4:31 AM

Details

Reviewers
melifaro
Group Reviewers
network
Commits
rS360344: pf: Improve input validation
Summary

If we pass an anchor name which doesn't exist pfr_table_count() returns
-1, which leads to an overflow in mallocarray() and thus a panic.

Explicitly check that pfr_table_count() does not return an error.

Reported-by: syzbot+bd09d55d897d63d5f4f4@syzkaller.appspotmail.com

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 30670
Build 28405: arc lint + arc unit

Event Timeline

LGTM, please see minor comments in the diff

sys/netpfil/pf/pf_ioctl.c
3058

I'm curious: it looks like pfr_get_tstats() uses totally separate routine to clear the statistics. Would it potentially make sense to use read lock to get the stats first and then clear the stats under wlock if the flag is specified?

3060

I'm wondering what will happen when n equals 0?
Do mallocarray() always return NULL?

This revision is now accepted and ready to land.Apr 22 2020, 8:41 PM
sys/netpfil/pf/pf_ioctl.c
3058

That would break the atomicity of the read+clear operation. We'd risk missing matches that happen between reading the stats and clearing them.

3060

That becomes a malloc(0, ...), so I believe we still allocate something, possibly rounded up to the smallest uma bucket size.

This revision was automatically updated to reflect the committed changes.