netbsd boot error: panic: UBSan: Undefined Behavior in /syzkaller/managers/netbsd-kubsan/kernel/sys/kern/subr_disk_mbr.c:LINE, member acces

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: b177c0ee Fix KUBSAN: the kernel size now exceeds the mappi..
git tree: netbsd
console output: https://syzkaller.appspot.com/x/log.txt?x=11045192e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=824b23e1f4b6c76b
dashboard link: https://syzkaller.appspot.com/bug?extid=56769dece0ec3e35731e

Unfortunately, I don't have any reproducer for this crash yet.

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+56769d...@syzkaller.appspotmail.com

[ 1.3496393] panic: UBSan: Undefined Behavior in
/syzkaller/managers/netbsd-kubsan/kernel/sys/kern/subr_disk_mbr.c:574:9,
member access within misaligned address 0xffff942d3de8c03c for type 'const
struct disklabel' which requires 8 byte alignment

[ 1.3663828] cpu0: Begin traceback...
[ 1.3689943] vpanic() at netbsd:vpanic+0x2aa sys/kern/subr_prf.c:336
[ 1.3788101] isAlreadyReported() at netbsd:isAlreadyReported
[ 1.3988418] HandleTypeMismatch.part.1() at
netbsd:HandleTypeMismatch.part.1+0xcc
[ 1.4088591] HandleTypeMismatch() at netbsd:HandleTypeMismatch+0x7b
sys/../common/lib/libc/misc/ubsan.c:408
[ 1.4288896] check_label_magic() at netbsd:check_label_magic+0x9f
sys/kern/subr_disk_mbr.c:574
[ 1.4389098] validate_label() at netbsd:validate_label+0x1b4
sys/kern/subr_disk_mbr.c:626
[ 1.4489214] look_netbsd_part() at netbsd:look_netbsd_part+0x3ce
sys/kern/subr_disk_mbr.c:526
[ 1.4689530] scan_mbr() at netbsd:scan_mbr+0x24a
sys/kern/subr_disk_mbr.c:234
[ 1.4789689] readdisklabel() at netbsd:readdisklabel+0x412
sys/kern/subr_disk_mbr.c:448
[ 1.4898443] dk_getdisklabel() at netbsd:dk_getdisklabel+0x192
sys/dev/dksubr.c:931
[ 1.5090177] dk_open() at netbsd:dk_open+0x456 sys/dev/dksubr.c:177
[ 1.5190333] sdopen() at netbsd:sdopen+0x114 sys/dev/scsipi/sd.c:543
[ 1.5290486] cdev_open() at netbsd:cdev_open+0xe5
sys/kern/subr_devsw.c:871
[ 1.5490803] spec_open() at netbsd:spec_open+0x3ad
sys/miscfs/specfs/spec_vnops.c:562
[ 1.5591508] VOP_OPEN() at netbsd:VOP_OPEN+0x113 sys/kern/vnode_if.c:298
[ 1.5791291] dkwedge_discover() at netbsd:dkwedge_discover+0xcf
sys/dev/dkwedge/dk.c:931
[ 1.5891449] sdattach() at netbsd:sdattach+0x53f sys/dev/scsipi/sd.c:362
[ 1.5991611] config_attach_loc() at netbsd:config_attach_loc+0x432
sys/kern/subr_autoconf.c:1604
[ 1.6191925] scsi_probe_bus() at netbsd:scsi_probe_bus+0xad8
scsi_probe_device sys/dev/scsipi/scsiconf.c:1035 [inline]
[ 1.6191925] scsi_probe_bus() at netbsd:scsi_probe_bus+0xad8
sys/dev/scsipi/scsiconf.c:413
[ 1.6292092] scsibus_discover_thread() at
netbsd:scsibus_discover_thread+0xdd scsibus_config
sys/dev/scsipi/scsiconf.c:320 [inline]
[ 1.6292092] scsibus_discover_thread() at
netbsd:scsibus_discover_thread+0xdd sys/dev/scsipi/scsiconf.c:285
[ 1.6392253] cpu0: End traceback...
[ 1.6392253] fatal breakpoint trap in supervisor mode
[ 1.6392253] trap type 1 code 0 rip 0xffffffff8021dddd cs 0x8 rflags
0x282 cr2 0 ilevel 0 rsp 0xffffbc80a5c02280
[ 1.6555812] curlwp 0xffff942c2bcce9c0 pid 0.28 lowest kstack
0xffffbc80a5bff2c0
Stopped in pid 0.28 (system) at netbsd:breakpoint+0x5: leave
db{0}>


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

[Kamil Rytarowski]

On 01.11.2019 18:58, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:    b177c0ee Fix KUBSAN: the kernel size now exceeds the
> mappi..
> git tree:       netbsd
> console output: https://syzkaller.appspot.com/x/log.txt?x=11045192e00000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=824b23e1f4b6c76b
> dashboard link:
> https://syzkaller.appspot.com/bug?extid=56769dece0ec3e35731e
>
> Unfortunately, I don't have any reproducer for this crash yet.
>


> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+56769d...@syzkaller.appspotmail.com
>
> [   1.3496393] panic: UBSan: Undefined Behavior in
> /syzkaller/managers/netbsd-kubsan/kernel/sys/kern/subr_disk_mbr.c:574:9,
> member access within misaligned address 0xffff942d3de8c03c for type
> 'const struct disklabel' which requires 8 byte alignment
>

Is this a false positive? We already patched our code to workaround
misalignment report. It uses memcmp(3) now.

static bool
check_label_magic(const struct disklabel *dlp, uint32_t diskmagic)
{
return memcmp(&dlp->d_magic, &diskmagic, sizeof(diskmagic)) == 0 &&
memcmp(&dlp->d_magic2, &diskmagic, sizeof(diskmagic)) == 0;
}

How to patch it more to appease the GCC8 sanitizer?

[Dmitry Vyukov]

My C-foo is not strong enough... need somebody with stronger C-foo.

On one hand even converting a pointer to an unaligned pointer is UB:

6.3.2.3/7
A pointer to an object type may be converted to a pointer to a
different object type. If the resulting pointer is not correctly
aligned for the referenced type, the behavior is undefined.

On the other hand, an int can be converted to an unaligned pointer,
which not a UB in itself:

6.3.2.3/5
An integer may be converted to any pointer type. Except as previously
specified, the
result is implementation-defined, might not be correctly aligned,
might not point to an
entity of the referenced type, and might be a trap representation.

And then it seems only dereferencing an unaligned pointer is UB, but
taking member address is not yet (?):

6.5.3.2/4
The unary * operator denotes indirection. If the operand points to a
function, the result is
a function designator; if it points to an object, the result is an
lvalue designating the
object. If the operand has type ‘‘pointer to type’’, the result has
type ‘‘type’’. If an
invalid value has been assigned to the pointer, the behavior of the
unary * operator is
undefined. 102)

102) Thus, &*E is equivalent to E (even if E is a null pointer), and
&(E1[E2]) to ((E1)+(E2)). It is
always true that if E is a function designator or an lvalue that is a
valid operand of the unary &
operator, *&E is a function designator or an lvalue equal to E. If *P
is an lvalue and T is the name of
an object pointer type, *(T)P is an lvalue that has a type compatible
with that to which T points.

Among the invalid values for dereferencing a pointer by the unary *
operator are a null pointer, an
address inappropriately aligned for the type of object pointed to, and
the address of an object after the
end of its lifetime.

This is based on N1570 standard version.

[Kamil Rytarowski]

Does this approach look good?

http://netbsd.org/~kamil/patch-00193-disklabel-unaligned-magic.txt

I have replaced pointer arithmetic with base + offset.

This code still boots for me.

[Kamil Rytarowski]

On 06.11.2019 13:37, Kamil Rytarowski wrote:
> Does this approach look good?
>
> http://netbsd.org/~kamil/patch-00193-disklabel-unaligned-magic.txt
>
> I have replaced pointer arithmetic with base + offset.
>
> This code still boots for me.
>

I've landed it. It should hopefully unblock syzkaller.

[Dmitry Vyukov]

I've consulted with some people and _presumably_ (to the degree one
can be sure about bitter corner cases of C/C++ :)) this is a correct
fix (and formally correct warnings from ubsan).
As 6.5.3.2/4 says, only &*p and &p[i] syntactic forms are defined as
special case of not being a dereference, but rather effectively an
address calculation. But &p->m is not. Thus it is interpreted as a
dereference that produces an lvalue and then taking address of that
lvalue. At the point of dereference we have UB. Your fix avoids the
dereference.

[Kamil Rytarowski]

On 07.11.2019 05:47, 'Dmitry Vyukov' via syzkaller-netbsd-bugs wrote:
> I've consulted with some people and _presumably_ (to the degree one
> can be sure about bitter corner cases of C/C++ :)) this is a correct
> fix (and formally correct warnings from ubsan).
> As 6.5.3.2/4 says, only &*p and &p[i] syntactic forms are defined as
> special case of not being a dereference, but rather effectively an
> address calculation. But &p->m is not. Thus it is interpreted as a
> dereference that produces an lvalue and then taking address of that
> lvalue. At the point of dereference we have UB. Your fix avoids the
> dereference.

Thank you for the analysis!