Re: [syzbot] WARNING: refcount bug in qrtr_node_lookup

On Thu, Sep 2, 2021 at 9:58 AM Paul Moore [off-list ref] wrote:

On Thu, Sep 2, 2021 at 12:13 AM Hillf Danton [off-list ref] wrote:

quoted

On Wed, 01 Sep 2021 19:32:06 -0700

quoted

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
UBSAN: object-size-mismatch in send4

================================================================================
UBSAN: object-size-mismatch in ./include/net/flow.h:197:33
member access within address 000000001597b753 with insufficient space
for an object of type 'struct flowi'
CPU: 1 PID: 231 Comm: kworker/u4:4 Not tainted 5.14.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: wg-kex-wg0 wg_packet_handshake_send_worker
Call Trace:
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0x15e/0x1d3 lib/dump_stack.c:105
 ubsan_epilogue lib/ubsan.c:148 [inline]
 handle_object_size_mismatch lib/ubsan.c:229 [inline]
 ubsan_type_mismatch_common+0x1de/0x390 lib/ubsan.c:242
 __ubsan_handle_type_mismatch_v1+0x41/0x50 lib/ubsan.c:271
 flowi4_to_flowi_common include/net/flow.h:197 [inline]

This was added in 3df98d79215a ("lsm,selinux: pass flowi_common instead of
flowi to the LSM hooks"), could you take a look at the UBSAN report, Paul?

Sure, although due to some flooding here at home it might take a day
(two?) before I have any real comments on this.

I'm looking quickly at this tonight after a long day so it's possible
I'm missing something, but in the original report if you look one step
before the backtrace above you see the caller is send4 in the
wireguard code, which starts off by creating it's own flowi4 variable
on the stack and then eventually passing it down to
flowi4_to_flowi_common() for use as the second parameter to
security_sk_classify_flow().  Because the wireguard code only
allocates a flowi4 and not a full flowi struct it seems like this
would explain the size mismatch warning (flowi is larger than flowi4
due to the union containing flowi6 as well as flowi4).

Off the top of my head it isn't clear to me if it is considered "safe"
to allocate just a flowi4 in this way, you may need to allocate a full
flowi; if none of the netdev folks reply we could always check the
other flowi4 users in the kernel.

Depending on the above, the fix may be either to adjust send4() to use
a full flowi, or to adjust flowi4_to_flowi_common() to use the
flowi_common struct at the top of the flowi4 struct instead of first
looking for the flowi struct and going from there.

-- 
paul moore
www.paul-moore.com