netlink: fix snl_writer and linear_buffer re-allocation logic

[kp]

Summary

- Use correct base pointer after re-allocation, it avoids buffer
  overflows.

- Maintain correct snl_writer.size, it avoids redundant memory
  allocation, e.g. need for ~1k bytes may end up with ~32k linear_buffer
  actually allocated.

It fixes pfctl regression at least for armv7 after addrule logic
migration to netlink:
  ffbf25951e7b ("pf: convert rule addition to netlink")
Add rule command creates bigger than default size netlink requests what
triggers re-allocation logic.

Test Plan

A quick shot is echo 'pass all' | pfctl -f-.

More testing:

• kyua test -k /usr/tests/sys/netlink/Kyuafile
• kldload pf dummynet ipdivert; kyua test -k /usr/tests/sys/netpfil/pf/Kyuafile

[igoro]

A concise version of the reasoning:

Netlink requests are fine if they fit default sizes, see #define SNL_WRITER_BUFFER_SIZE 256 and #define SCRATCH_BUFFER_SIZE 1024.

But kind of undefined behavior doors are open when it's time to allocate a new linear_buffer. The nw->base is not maintained correctly (it gets shifted by lb->offset) what creates buffer overflow situation, but as long as nw->size also is not maintained correctly a linear_buffer has bigger than needed but enough memory allocated for such overflows. The lb behind is constantly asked for re-allocation of nw->size doubled, and while nw->size stays unchanged (256), lb is always asked for +512. And when nw gets more than 512 bytes written (this is the case for pf add rule command) we start doing read from the same buffer and write to the same buffer, with overlapping of src and dst memory ranges.

Therefore, the outcome depends on $TARGET.$TARGET_ARCH. For instance, from my testing point of view:

• aarch64 manages to end up with a correct content after that overlapping memcpy(), even if linear_bufferS behind the nw are total mess
• but armv7 ends up with a distorted content, i.e. wrong netlink request is sent, nl parser complains and responds back with an error, e.g. Invalid attr 0xdcc2f200 type 0 len 0

Currently I know about a single production need in this patch -- to fix pfctl for armv7.

[kp]

sys/netlink/netlink_snl.h

1045–1049

It looks really odd to overwrite new_base here, given that we've assigned the new allocation to it on line #1039.
It's not wrong as such, but it does look very strange.

It feels like there's an incorrect amount of abstraction between the snl and lb layers (either too much or too little, I'm not sure which yet).

I'd be tempted to avoid the really odd look by doing this:

if (snl_allocz(nw->ss, new_size) == NULL) {
        nw->error = true;
        return (false);
}
nw->size = new_size;

void *new_base = nw->ss->lb->base;

[igoro]

Yes, we have to break the Demeter here for the sake of the bug fix. Some refactoring could take place here, but I guess it’s a separate project.

I like the potentially less oddity change. Please, find the updated patch.

[kp]

Commandeer to close.

[kp]

Close manually. Committed in https://cgit.freebsd.org/src/commit/?id=0c511bafdd5b309505c13c8dc7c6816686d1e103