On Fri, Feb 28, 2025 at 1:35 AM Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> wrote: > > On Thu, Feb 27, 2025 at 04:55:06PM -0800, Jeff Xu wrote: > > Hi Lorenzo, > > > > On Tue, Feb 25, 2025, 9:43 PM Lorenzo Stoakes > > > > > > > > 2. Tests - could you please add some tests to assert that mremap() fails > > > > > for VDSO for instance? You've edited an existing test for VDSO in x86 to > > > > > skip the test should this be enabled, but this is not the same as a self > > > > > test. And obviously doesn't cover arm64. > > > > > > > > > > This should be relatively strightforward right? You already have code > > > > > for finding out whether VDSO is msealed, so just use that to see if you > > > > > skip, then attempt mremap(), mmap() over etc. + assert it fails. > > > > > > > > > > Ideally these tests would cover all the cases you've changed. > > > > > > > > > It is not as easy. > > > > > > > > The config is disabled by default. And I don't know a way to detect > > > > KCONFIG from selftest itself. Without this, I can't reasonably > > > > determine the test result. > > > > > > Please in future let's try to get this kind of response at the point of the > > > request being made :) makes life much easier. > > > > > There might be miscommunication ? > > This version is the first time you ask about adding a self test. > > Yeah I thought I'd been clear but this might _very well_ have been me not > having been, so apologies if so. > > I mean 'make sure it's tested' is an overloaded term right? So fact you've > tested on android, chromeos, etc. implies 'tested', but also self > tests/kunit/whatever. > > > > > IIRC, you had comments about providing the details of what tests I did, in V4. > > As a follow-up, I added a test-info section in the cover letter of V5 > > Thanks. Appreciate that! And this really does point towards a > miscommunication on my part, will try to be super explicit in future. > > > > > Though I have thought about adding a selftest since the beginning, > > there are two problems: > > 1> This config is by default disabled, > > 2> No good pattern to check KCONFIG from selftest. > > Yeah agreed, it's a TOTAL pain. > > I wish we had a better way of doing this. Maybe a self-volunteering > exercise (*goes to write on writeboard :P*) > > > > > > > > > So I think it is easy actually. As I say here (perhaps you missed it?) you > > > literally already have code you added to the x86 test to prevent test > > > failure. > > > > > > So you essentially look for [vdso] or whatever, then you look up to see if > > > it is sealed, ensure an mremap() fails. > > > > > This suggestion doesn't test the core change of this series, which is > > to enable mseal for vdso. > > Right, and thinking about it, what does this test? Just that mseal works > right? > > It's sort of implicit that, if a VMA is sealed, the seal should work (or > rather, tested in mseal tests themselves, rather than mseal system > settings). > > > > > When the vdso is marked with "sl", mremap() will fail, that's part of > > the mseal() business logic and belongs in mseal_test. The mseal_test > > already covers the mseal, and this series doesn't change mseal. > > > > As for the "sl", as I responded in the "refactor mseal_test" [1] , it > > will be part of the verifying step: > > > > [1] https://lore.kernel.org/all/CABi2SkUv_1gsuGh86AON-xRLAggCvDqJMHxT17mGy-XutSTAwg@xxxxxxxxxxxxxx/ > > OK cool thanks. I will look at this when I can, I'm just snowed under > pre-LSF and have been sick so backlog, backlog as discussed off-list. But > it's not been forgotten (whiteboard etc. etc.) > Ya, no worry about that review, please take time to recover, I will wait. And appreciate your time and take priority for reviewing this series. > > > > > Of course this doesn't check to see if it _should_ be enabled or not. I'm > > > being nice by not _insisting_ we export a way for you to determine whether > > > this config option is enabled or not for the sake of a test (since I don't > > > want to hold up this series). > > > > > > But that'd be nice! Maybe in a > > > /sys/kernel/security endpoint... > > > > > > > > > ...but I think we can potentially add this later on so we don't hold things > > > up here/add yet more to the series. I suspect you and Kees alike would > > > prioritise getting _this series_ in at this point :) > > > > > > You could, if you wanted to, check to see if /proc/config.gz exists and > > > zgrep it (only there if CONFIG_IKCONFIG_PROC is set) and assert based on > > > that, but you know kinda hacky. > > > > Ya, it is hacky. None of the existing selftest uses this pattern, and > > I'm not sure /proc/config.gz is enabled in the default kernel config. > > Yeah and I'm not sure I even like my hacky suggestion here, I mean let's be > honest, it sucks. > > > > > One option is to have ChromeOS or Android to maintain an out-of-tree > > test, since the configuration will be enabled there. > > Nah haha, though of course you can do what you want. Really want something > upstream. > > > > > Option two is to create a new path: > > tools/testing/selftests/sealsysmap. Then, add > > CONFIG_SEAL_SYSTEM_MAPPING=y to the config file and add a selftest to > > this path. This seems to be the preferred way by selftest, but we need > > a new dir for that. > > OK I like option 2 let's do this. > > But let's call it mseal_system_mappings (yes I"m being nitty again :) I > really want to try to _explicitly_ say 'mseal' because we have other forms > of sealing. > Sure. If long path names aren't a problem, I will use mseal_system_mappings, otherwise mseal_sysmap. > Not your fault, but we overload terms like _crazy_ in mm and need to be > cautious as not to confuse vs. e.g. memfd seals. > > > > > > Option three is to add a self-test when we have a process-level opt-in > > solution. This would allow the test to deterministically know whether > > the vdso should be sealed or not. > > Yeah one for future. > > > > > > > > > But anyway, FWIW I think it'd be useful to assert that mremap() or munmap() > > > fails here for system mappings for the sake of it. I guess this is, in > > > effect, only checking mseal-ing works right? But it at least asserts the > > > existence of the thing, and that it behaves. > > > > > > Later on, when you add some way of observing that it's enabled or not, you > > > can extend the test to check this. > > > > I think it is best that we don't add a test that doesn't actually > > check the code change. Do you think one of the above three options > > works ? maybe the second option (with a new path) ? > > Yeah I actually agree on reflection. And yes agreed option 2 is great, > thanks! > > > > > In any case, I think the risk is low, and the code changes are quite > > simple, and fully tested. > > Yeah indeed, but I'd really like _something_ if possible. If option 2 is > relatively quick let's get that sorted out! > Great ! I will work on option 2. Thanks -Jeff
