Bug 185617 - pfctl(8): armv6: "pfctl -s state" crashes on BeagleBone Black due to unaligned access
Summary: pfctl(8): armv6: "pfctl -s state" crashes on BeagleBone Black due to unaligne...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: arm Any
: Normal Affects Only Me
Assignee: freebsd-pf (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-09 21:40 UTC by guyyur
Modified: 2018-05-30 07:03 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description guyyur 2014-01-09 21:40:02 UTC
I am running 10.0-RC1 arm.armv6 on the BeagleBone Black.
The "pfctl -s state" command is crashing when trying to print the
second entry.

struct pfsync_state has a size that is not divisiable by 4 leading to the
second entry in the returned state array not being aligned.  This is fine when
accessing the entry as a struct pfsync_state pointer since the struct has the
__packed attribute and on arm unaligned access will be used.  When print_host
is called it receives a pf_addr struct pointer &nk->addr[1] which is not
aligned on 4 bytes for the second entry and since the struct is not __packed
it will be accessed using word load instructions which will trigger an
unaligned access fault and pfctl will exit with bus error.

(gdb) bt
#0  print_host (addr=0x2085a11a, port=7660, af=2 '\002', opts=1024) at /usr/src/sbin/pfctl/pf_print_state.c:178
#1  0x00021c4c in print_state (s=0x2085a0f2, opts=1024) at /usr/src/sbin/pfctl/pf_print_state.c:236
#2  0x0000c664 in pfctl_show_states (dev=<value optimized out>, iface=0x0, opts=1024) at /usr/src/sbin/pfctl/pfctl.c:1095

sizeof(struct pfsync_state_key) is 36
sizeof(struct pfsync_state_peer) is 32
sizeof(struct pf_addr) is 16
sizeof(struct pfsync_state) is 242

Fix: 

A quick workaround is for print_state to copy the pf_addr in pfsync_state_key
to a pf_addr struct on the stack and pass it to print_host.

Another possibility is to make sure pfsync_state is aligned on at least
4 bytes (8 preferred for the u_int64_t id) or create a new aligned struct
for the DIOCGETSTATE and DIOCGETSTATES ioctls to separate between the
pfsync_state as protocol data and the info returned by the ioctls.
Changing pfsync_state size will break KBI and the pfsync protocol.
How-To-Repeat: pf running on an arm host, make sure there is more than one active connection.
Run: pfctl -s state
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2014-05-04 06:03:54 UTC
Responsible Changed
From-To: freebsd-arm->freebsd-pf

Over to maintainer(s).
Comment 2 guyyur 2015-03-17 14:24:46 UTC
Patch that breaks backward compatibility by separating the pfsync and pfioc state structures and uses host order for pfioc fields except for id and creatorid.

pfsync_state_import is duplicated to pfioc_state_import.
pfioc_state_export is duplicated to pfsync_state_export.
pfsync_alloc_scrub_memory is duplicated to pfioc_alloc_scrub_memory.


https://github.com/guyyur/freebsd-src_patches/blob/master/pfctl_arm_segbus__ver2_part1.patch
Comment 3 commit-hook freebsd_committer freebsd_triage 2016-01-30 22:03:31 UTC
A commit references this bug:

Author: ian
Date: Sat Jan 30 22:03:15 UTC 2016
New revision: 295086
URL: https://svnweb.freebsd.org/changeset/base/295086

Log:
  Make pfctl(8) work on strict-alignment platforms, by copying a pair of
  embedded structures out of a packed, unaligned struct into local copies
  on the stack which are aligned.

  The original patch to do this was submitted by Guy Yur <guyyur@gmail.com>,
  and this is conceptually the same change, but restructured with the
  #ifndef __NO_STRICT_ALIGNMENT wrapper, similar to how the same issue is
  handled in the kernel pf code.

  PR:		185617
  PR:		206658

Changes:
  head/sbin/pfctl/pf_print_state.c
Comment 4 Ian Lepore freebsd_committer freebsd_triage 2016-01-30 22:09:41 UTC
While r295086 fixes the crash on strict-alignment platforms, I am not going to close this bug.  Instead, I leave it to the networking gurus to evaluate the patch referenced in comment #2.  I don't know enough about pf and networking in general to evaluate the tradeoffs in restructuring the, umm, structures.
Comment 5 Eitan Adler freebsd_committer freebsd_triage 2018-05-28 19:46:33 UTC
batch change:

For bugs that match the following
-  Status Is In progress 
AND
- Untouched since 2018-01-01.
AND
- Affects Base System OR Documentation

DO:

Reset to open status.


Note:
I did a quick pass but if you are getting this email it might be worthwhile to double check to see if this bug ought to be closed.
Comment 6 guyyur 2018-05-29 20:40:45 UTC
This bug can be closed.
Comment 3 contains the commit reference that fixes this bug.

It was left open to evaluate restructuring pfsync and pfioc
but the patches are now out of date.
They can be resubmitted independently from this bug.