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