i2c-aspeed testing with kunit fake and syzkaller

[Dmitry Vyukov]

Hi i2c/aspeed-related people,

Some time ago Brendan and myself did an experiment of testing the
i2c-aspeed driver in a qemu VM (without any hardware/emulation) using
kunit-based fake hardware. The fake allowed us to get 10 i2c devices
successfully probe and appear in /dev/. Which in turn allowed us to
test the actual driver with syzkaller.
I will just leave here some details and traces as FYI and for future
reference. I think this is an important precedent in general.

The kernel branch is here:
https://github.com/dvyukov/linux/commits/kunit-i2c-stub2
This is one of kunit branches with this commit on top:
https://github.com/dvyukov/linux/commit/c9c1706611fdc49679c5bf5bff1e147ab7c7aa79
This contains lots of hacks to get the fake working on x86 and leave
persistent devices initialized (rather than shutdown them after
unit-testing).

This is the kernel config:
https://gist.githubusercontent.com/dvyukov/72a31dad43b5687e6ffdcdb4cb342175/raw/24b706326e2d9202789fece5bfc17355aadda411/gistfile1.txt
(some i2c configs + a bunch of debugging configs).

If you boot this kernel with the config in qemu, you should see /dev/i2c-{0,10}.

Probably any syzkaller commit will work, but I just testing on this one:
https://github.com/google/syzkaller/tree/2ab6c4a4571d4170b0009a5b14b9789b016814b6
Here is syzkaller config I used:
https://gist.githubusercontent.com/dvyukov/d0a3c0e9b44cc7d445b36a8a50404ed8/raw/5f6aab34c6515cc4e96dac6cf3a98aaaf5e21b58/gistfile1.txt

Here is kernel coverage we achieved as the result of fuzzing:
https://drive.google.com/file/d/1JdNshmCWI3crJ3BuVo6uvRuHMBrwH5eB/view?usp=sharing

For crashes, I've got 3 different task hangs:

INFO: task hung in i2c_transfer
https://gist.githubusercontent.com/dvyukov/25e98a59b643bc868c7489f566ed9eab/raw/67b717488ae9af381a5b0615e5ddf2d226369d3c/gistfile1.txt

INFO: task hung in i2c_smbus_xfer
https://gist.githubusercontent.com/dvyukov/7ca87fea1b397d46e3393227a34595b9/raw/f5385f025abfc3ad2e91fd4f2616e58c7de8804e/gistfile1.txt

INFO: task hung in aspeed_i2c_master_xfer
https://gist.githubusercontent.com/dvyukov/dbc51e3ea7034f255c8fd1c99965d91a/raw/a5165e4e6c3b41d593d02fc22996104e468cfab3/gistfile1.txt

And some memory corruptions (non-thread-safe kunit data structures?):

KASAN: use-after-free Read in aspeed_i2c_fake_write_command_reg
https://gist.githubusercontent.com/dvyukov/6ca5bca5cf7a40a93636360635a4672c/raw/5ad0e7b33907f4b0db102b74c2a11237df612462/gistfile1.txt

KASAN: use-after-free Read in __of_find_property
https://gist.githubusercontent.com/dvyukov/95ed93a39d38fd215c141b58e5d3ec0f/raw/ea7f2346982a31ab16fca2850f91e1ef11faf687/gistfile1.txt

general protection fault in mock_do_expect
https://gist.githubusercontent.com/dvyukov/8c2e3def6775d39cb7496b56a77cbcd8/raw/5778337009e21397b484aafe5b24c87ebe64b63f/gistfile1.txt

There are probably a number of things that can be improved in kunit
platform mock, the fake driver and syzkaller i2c coverage, but this is
a starting point.

Thanks

[Brendan Higgins]

+kuni...@googlegroups.com

The mock_do_expect thing is definitely an issue, but also, not
upstream, so I will fix it before I send that out for review.

As for aspeed_i2c_master_xfer(); the issue appears to be that we do a
wait_for_completion_timeout, which should be interruptible. So I
should probably submit a fix for that.

I was less confident in i2c_transfer, i2c_smbus_xfer, and
__of_find_property; nevertheless:

i2c_transfer doesn't actually look like a bug. It looks like the
faking framework aborted, resulting in a condition which can never
happen in the i2c-core in the real world. Same goes for
i2c_smbus_xfer.

The issue in __of_find_property appears to be that it hits a property
which was not properly initialized when the device_node was
initialized with of_node_init()[1]. I am not an expert in how the
device tree stuff works, but I would expect that of_node_init() should
fully initialize the device_node passed to it. So that might be a
legitimate issue.

I'll take a closer look at i2c_transfer, and i2c_smbus_xfer and see if
I missed anything.

Cheers!

[Joel Stanley]

On Tue, 8 Oct 2019 at 14:44, Dmitry Vyukov <dvy...@google.com> wrote:
>
> Hi i2c/aspeed-related people,
>
> Some time ago Brendan and myself did an experiment of testing the
> i2c-aspeed driver in a qemu VM (without any hardware/emulation) using
> kunit-based fake hardware. The fake allowed us to get 10 i2c devices
> successfully probe and appear in /dev/. Which in turn allowed us to
> test the actual driver with syzkaller.
> I will just leave here some details and traces as FYI and for future
> reference. I think this is an important precedent in general.

Wow, this is extensive. It will take me some time to digest it.

Thanks for sending it along.

Cheers,

Joel

[Dmitry Vyukov]

On Wed, Oct 9, 2019 at 8:48 AM Joel Stanley <jo...@jms.id.au> wrote:
>
> On Tue, 8 Oct 2019 at 14:44, Dmitry Vyukov <dvy...@google.com> wrote:
> >
> > Hi i2c/aspeed-related people,
> >
> > Some time ago Brendan and myself did an experiment of testing the
> > i2c-aspeed driver in a qemu VM (without any hardware/emulation) using
> > kunit-based fake hardware. The fake allowed us to get 10 i2c devices
> > successfully probe and appear in /dev/. Which in turn allowed us to
> > test the actual driver with syzkaller.
> > I will just leave here some details and traces as FYI and for future
> > reference. I think this is an important precedent in general.
>
> Wow, this is extensive. It will take me some time to digest it.
>
> Thanks for sending it along.
>
> Cheers,
>
> Joel
>
>
> >
> > The kernel branch is here:
> > https://github.com/dvyukov/linux/commits/kunit-i2c-stub2
> > This is one of kunit branches with this commit on top:
> > https://github.com/dvyukov/linux/commit/c9c1706611fdc49679c5bf5bff1e147ab7c7aa79
> > This contains lots of hacks to get the fake working on x86 and leave
> > persistent devices initialized (rather than shutdown them after
> > unit-testing).

One thing I should have pointed to is the actual "hardware fake":
https://github.com/dvyukov/linux/blob/d154ffa80dff44c8b941814d700b22e6cf4c23c2/drivers/i2c/busses/i2c-aspeed-fake.c

Brendan, in this mode kunit prints whole lot of messages re
missed/failed expectations, etc. It probably should have a special
mode for platform mock when it does not bother about expectations, at
least doesn't print anything.

[Brendan Higgins]

That's a good point. I will put it on the TODO list. Nevertheless, I
am afraid there are a lot of things ahead of it (in regard to KUnit).
(People should let me know if they find this interesting and I will
try to move it up the priority list.)

[Dmitry Vyukov]

Another problem popped up. It turns out kunit can produce assertion
failures, but syzkaller wasn't catching them as they have syntax
different from any other kernel bugs/oopses.
Eventually kunit assertion handling code triggered "BUG: sleeping
function called from invalid context" and "WARNING:
syz-executor.4/3822 still has locks held!", so I looked at the log and
noticed the kunit assertion as well. Full output is here:
https://gist.githubusercontent.com/dvyukov/19326832ad245aff33b6696b4e97f5a3/raw/917549be0e97ee90b6e679e4d9a75190a77d1932/gistfile1.txt

It would be useful to give some explicit thought to the way kunit
reports all issues and converge on some easier-to-parse unified
format.
How kernel notifies about bugs/oopses is effectively a public
interface for any kind of kernel testing systems -- they need to
understand that the kernel has self-detected something bad and to
parse that message to some degree. Parsing kernel crash output is lots
of fun:
https://github.com/google/syzkaller/blob/master/pkg/report/linux.go

Based on our experience with syzbot, we converged on the following
crash identity as the most useful/stable: "TOOL: bug-type in
function-name".
For the current kunit message:

kunit aspeed-i2c-test: ASSERTION FAILED at
drivers/i2c/busses/i2c-aspeed-fake.c:87

(1) It's hard to understand that it's in fact a bug. If we look for
"kunit", then any informational kunit message may be confused with a
bug. Looking for "ASSERTION FAILED" may be better, but it does not
capture all other possible bug types kunit may detect (or will fail if
you decide to rephrase "ASSERTION FAILED").
(2) file/line number is not too good as bug identity, consider we pull
new code, and now suddenly it becomes a different line. Is it the old
bug? Or a new one? Function name works better as bug identity.

Brendan, could you do some common recognizable prefix for all
kunit-detected bugs? (e.g. "KUNIT:" or something)
Also it would be good to have function name, not necessary on the
first line, syzkaller can extract it from subsequent lines too. But
ideally it's unambiguously recognizable with some regexp (not not
confuse it with some other unrelated output).

> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/CAFd5g45xQvYozyuoFKXXcP6FFBZ6ZOVR-%3DgQ8J7_MEwWG_QmOg%40mail.gmail.com.