search

lkrg-org / lkrg

Linux Kernel Runtime Guard
https://lkrg.org
Other
410 stars 72 forks source link

Harden LKRG's sysctl handlers #204

Open solardiz opened 2 years ago

solardiz commented 2 years ago

Right now, LKRG's sysctl handlers open/close r/w access at the outer layer, regardless of whether the access is a write or a read. We should either skip that for reads or use temporary (not on r/o page) variables in the outer layer so that the open/close can be moved to inside the existing if block.

Also, the min/max variables used in the sysctl table should be made const, and/or the values checked against the corresponding constants before being put into the r/o page.

solardiz commented 2 years ago

Also, should fix handling of races between 2+ concurrent sysctl updates, where with current code one of these could set the page back to r/o and another then crash on writing into the page that it had opened r/w access for.

solardiz commented 2 years ago

use temporary (not on r/o page) variables in the outer layer so that the open/close can be moved to inside the existing if block.

I think we'll need to do that, because as seen in backtrace in https://github.com/lkrg-org/lkrg/issues/191#issuecomment-1159913630 the call to proc_dointvec_minmax can sleep (perhaps that was a debugging build deliberately tagging all potentially-sleeping memory allocations, but anyway at least theoretically the issue is presumably there).

I happen to have implemented this today for log_level for an unrelated reason. We could do similar for them all.

solardiz commented 2 years ago

While at it, also look into and clean up usage of signed vs. unsigned integers vs. printf formats. Right now, we use a weird mix.

solardiz commented 2 years ago

I think before proceeding to fix these issues, we should change most of the sysctl handlers to use a shared function in place of their bodies. So that we don't introduce even more copy-pasted-edited code.