On 12.12.2018 12:50, Maxime Villard wrote:
> Le 12/12/2018 à 11:01, Siddharth Muralee a écrit :
>> Hello,
>> I have attached a patch which adds the Kernel Code Coverage(KCov)
>> to the NetBSD kernel.
>> This is one of the foundational steps to get Syzkaller (Kernel Fuzzer)
>> fully functional on NetBSD.
>>
>> The option can be enabled by uncommenting the following lines in
>> amd64/GENERIC
>> makeoptions KCOV=1
>> options KCOV
>>
>> The patch also contains a man page and a ATF test for the same.
>>
>> Please let me know if there are any issues with the same.
>
> What I can think of:
>
> 1. Please use proper KNF in the code example of the man page, and also in
> the ATF test
>
> 2. Don't use capital letters for bool, just !pmap_extract
>
> 3. kd_alloc, kd_lookup, etc, should be static, there shouldn't be any
> shared function or variable
>
> 4. The __sanitizer_cov_trace_pc() prototype should be in kcov.c, and the
> kcov_exit() prototype around "#ifdef _KERNEL" (since it is included
> from
> userland)
>
> 5. You probably don't need the HAVE_GCC in Makefile.amd64, also it
> shouldn't
> be in Makefile.amd64 but rather in bsd.sys.mk (take KLEAK as example)
>
> 6. The driver is not modulable, so there's no point making a kcov
> directory
> in sys/modules/
>
> 7. I would drop the debugging code, there's no interest, it is a very
> small
> and basic driver, so nothing particular to debug
>
> 8. I would call the include kcov_ioctl.h, and I would put it in sys/dev/
>
> 9. By the way, the values used in the ioctl are confusing IMO, I would
> use
> KIOSETBUFSIZE -> KCOV_IOC_SETBUFSIZE
> KIOENABLE -> KCOV_IOC_ENABLE
> KIODISABLE -> KCOV_IOC_DISABLE
> Otherwise it sounds like "Kernel I/O", which really has nothing to do
> with kcov
>
If we are going to rename OpenBSD macros, I propose to switch to the
Linux ones:
KCOV_INIT_TRACE
KCOV_ENABLE
KCOV_DISABLE
https://github.com/torvalds/linux/blob/6f0d349d922ba44e4348a17a78ea51b7135965b1/include/uapi/linux/kcov.h
This way we will be more source-code compatible with the original
implementation.
> 10. In kern_exit.c you probably need a #include "opt_kcov.h"
>
> 11. What does 'nmemb' stand for? I would use something like 'nentries', it
> sounds more explicit. Also the 'kd' name sounds wrong to me, this
> looks
> like 'Kernel Descriptor', I would just use 'kcov', eg,
> 'kcov_lookup_pid'
>
> 12. kcovopen() seems racy to me, if two processes open /dev/kcov at the
> same time TAILQ_INSERT_TAIL() will clobber pointers; same problem in
> kd_lookup_pid(), and basically the rest
>
> 13. Given that kd_buf[0] seems to be an index, I would declare it as a
> struct { uint64_t n; uintptr_t pc[]; }
>
> 14. Please use consistent types, eg in kd_alloc() it should be uintptr_t
> and not vaddr_t
>
> 15. Keep in mind that __sanitizer_cov_trace_pc() is a big hot path, and
> that
> everything called from there should be optimized for performance. So I
> would add a __predict_false() on 'cold', and I would bring together
> the
> 'kd_mode', 'kd_pid' and 'kd_entry' fields to optimize the lookup.
>
> In short, I think that for really simple drivers like that, it is better to
> write one from scratch, with clear names, no bugs, and limited slowdown,
> rather than taking code from the outside (here OpenBSD, which they
> themselves
> took from Linux).
>
> Maxime
Upstream (dvyukov) suggested to evaluate whether to use pointers always
in 64bit types (uint64_t), as this way programs will be easier to reuse
in 32bit mode on 64-bit kernel mode.
Fuzzing 32-bit-on-64-bit-kernel is likely something we want.
We have a similar issues with the ktrace protocol in
32-bit-in-64-bit-kernel.
Attachment:
signature.asc
Description: OpenPGP digital signature