On 7/20/20 10:20 PM, Christoph Hellwig wrote: > On Mon, Jul 20, 2020 at 10:15:37PM -0700, Guenter Roeck wrote: >>>> - if (CHECK_DATA_CORRUPTION(uaccess_kernel(), >>>> + if (CHECK_DATA_CORRUPTION(!uaccess_kernel(), >>>> >>>> How does this work anywhere ? >>> >>> No, that is the wrong check - we want to make sure the address >>> space override doesn't leak to userspace. The problem is that >>> armnommu (and m68knommu, but that doesn't call the offending >>> function) pretends to not have a kernel address space, which doesn't >>> really work. Here is the fix I sent out yesterday, which I should >>> have Cc'ed you on, sorry: >>> >> >> The patch below makes sense, and it does work, but I still suspect >> that something with your original patch is wrong, or at least suspicious. >> Reason: My change above (Adding the "!") works for _all_ of my arm boot >> tests. Or, in other words, it doesn't make a difference if true >> or false is passed as first parameter of CHECK_DATA_CORRUPTION(), except >> for nommu systems. Also, unless I am really missing something, your >> original patch _does_ reverse the logic. > > Well. segment_eq is in current mainline used in two places: > > 1) to implement uaccess_kernel > 2) in addr_limit_user_check to implement uaccess_kernel-like > semantics using a strange reverse notation > > I think the explanation for your observation is how addr_limit_user_check > is called on arm. The addr_limit_check_failed wrapper for it is called > from assembly code, but only after already checking the addr_limit, > basically duplicating the segment_eq check. So for mmu builds it won't > get called unless we leak the kernel address space override, which > is a pretty fatal error and won't show up in your boot tests. The > only good way to test it is by explicit injecting it using the > lkdtm module. > Guess I lost it somewhere. Are you saying the check was wrong all along and your patch fixed it ? Thanks, Guenter
