Differential D47027
jail: Avoid dereferencing a potentially null pointer in kern_jail_get() Authored by markj on Oct 9 2024, 3:45 PM.
Details The bin/hostname test triggers this crash for me in a riscv VM running Pass an empty string rather than a NULL pointer, since we otherwise end Sponsored by: Innovate UK
Diff Detail
Event TimelineComment Actions Shouldn't we simply omit outputting ip4.addr (resp., ipv6.addr) if pr->pr_addrs[PR_INET] (resp., pr->pr_addrs[PR_INET6]), is NULL? Comment Actions vfs_setopt_part() has the side effect of setting the key length to 0, even though there's nothing to copy. I presume that we want to preserve that behaviour. Comment Actions My remark was too offhand, a little bit of code reading tells me that, in fact, the protocol is that we are passed options userland is interested in, which we then have to fill. Additionally, we test that there were no unknown parameters passed at the end, which requires marking each known parameter beforehand (which vfs_setopt_part() does). Comment Actions IIRC, I tested that on amd64 and all looks good. I'm not sure how that will behave on RISCV but on amd64 pr->pr_addrs[PR_INET]->pr_ip will not panic if pr->pr_addrs[PR_INET] == NULL. Comment Actions Ahh, right, this isn't a null pointer dereference in practice. It crashes in my case because on CHERI-riscv at least, the capability passed to vfs_setopt_part() needs to have its bounds set. I'm not sure if this is expected after all. Comment Actions This code is fine, if a bit suspicious-looking. The cheri-riscv emulator I was using is strict and raises an exception when applying bounds to the NULL-derived pointer, hence a panic, but it doesn't make sense to modify FreeBSD here IMHO. Sorry for the noise. Comment Actions Sorry I'm late to the game. This actually seems like something should go in. The null dereference was introduced in 500f82d6c32ed, replacing earlier code that was unintuitive but notably didn't need to dereference that pointer. Unfortunately, the fix didn't include a null check, leading to a statement that has one non-null-checked use of the pointer, followed immediately by a properly checked use (because that one predated 500f82d6c32ed). Comment Actions But, the non-null-checked use of the pointer is harmless, as nothing actually dereferences it. Because pr_ip is an array, the value passed to vfs_setopt_part() is the value of the pointer plus the offset of the array within the structure being pointed to. I only noticed this because I was running tests under CHERI-RISCV in a mode where this admittedly dubious code pattern causes a trap from an instruction which sets bounds on a pointer. The solution was to switch to a more conservative non-trapping behaviour by default in the emulator. If you still prefer to have an explicit null check, then I'm happy to commit the patch, but it doesn't fix any real problem as far as I can see. Comment Actions Yeah, I saw that mentioned in the discussion, but it didn't really sink in. Thanks for spelling it out ;-). Sure, I'm fine with things staying as they are. |