10.0-RC1, armv6: "pfctl -s state" crashes on BeagleBone Black due to unaligned access

[Guy Yur]

On Fri, Jan 10, 2014 at 1:38 AM, John-Mark Gurney <jmg at funkthat.com> wrote:
> Guy Yur wrote this message on Fri, Jan 10, 2014 at 01:04 +0200:
>> On Fri, Jan 10, 2014 at 12:26 AM, John-Mark Gurney <jmg at funkthat.com> wrote:
>> > Guy Yur wrote this message on Fri, Jan 10, 2014 at 00:17 +0200:
>> >> On Thu, Jan 9, 2014 at 12:42 PM, Gleb Smirnoff <glebius at freebsd.org> wrote:
>> >> >   Guy,
>> >> >
>> >> > On Sat, Jan 04, 2014 at 03:06:02PM +0200, Guy Yur wrote:
>> >> > G> I am running 10.0-RC1 arm.armv6 on the BeagleBone Black.
>> >> > G> The "pfctl -s state" command is crashing when trying to print the
>> >> > G> second entry.
>> >>
>>
>> > Ok, that makes sense...  so, either we mark struct pf_addr as __packed,
>> > or we do some nasty stuff, like the following in print_host:
>> > struct {
>> >         struct pf_addr a
>> > } *uaddr __packed;
>> >
>> > uaddr = addr;
>> > aw.v.a.addr = uaddr->a;
>> >
>> > it's not pretty, but I believe it would work...
>>
>> For performance reasons, I don't think pf_addr should be marked as __packed.
>>
>> I attached the changes I am now using in print_state() since there is
>> no need to copy
>> the full pfsync_state, only pf_addr.
>> I converted sk and nk from pointers to structs on the stack and using
>> struct copy.
>> pf_addr is 16 bytes.
>
> Did you look at using the above trick?
>
> Since we are iterating over a list, that'll be a lot of copies, plus,
> I'm not sure that your fix will be guaranteed to work for ever.. since
> there isn't a requirement that the copy happens w/ bcopy/memcpy or some
> other copy routine that assumes things might not be aligned...
>

Right.
The correct fix would be to have a separate struct for the ioctl that can
be aligned as Gleb suggested.
I will try to prepare and test such changes.
If new ioctls are added, the KBI can also be preserved.
pfsync_state_export is used by if_pfsync.c and pf_ioctl.c so there will
be duplicated code even if reusing the old ioctls with the new struct.

> Specificly these:
> -               sk = &s->key[PF_SK_STACK];
> -               nk = &s->key[PF_SK_WIRE];
> +               sk = s->key[PF_SK_STACK];
> +               nk = s->key[PF_SK_WIRE];
>
> since s->key is already assumed to be aligned, a future compiler could
> be smart enough to say, I'm not going to use the stack..  That
> would/could happen if print_host's addr arg grew a const which it
> could...
>

I thought that because s itself is __packed and key is an array inside s
the __packed will apply to it too,  since the disassembly showed clang
chose to do a memcpy, I don't know if that is true or not.
An explicit bcopy is safer.

I see that the function already does a bcopy of the u_int64_t id field
so it has some assumption that the structure might not be aligned.

If a new always aligned structure is used for the ioctl, the bcopy of id
can also be avoided.

> Also, I just realized that some of the lines modify sk (setting port),
> but you don't write those modifications back to s->key[PF_SK_STACK]...
>

The print function doesn't need to modify s->key, the port changes are
only for passing to print_host.


> --
>   John-Mark Gurney                              Voice: +1 415 225 5579
>
>      "All that I will do, has been done, All that I have, has not."

Regards,
Guy