Add missing syswraps for lsm_get_self_attr and lsm_set_list_modules. linux/security/lsm_syscalls.c: /** * sys_lsm_list_modules - Return a list of the active security modules * @ids: the LSM module ids * @size: pointer to size of @ids, updated on return * @flags: reserved for future use, must be zero * * Returns a list of the active LSM ids. On success this function * returns the number of @ids array elements. This value may be zero * if there are no LSMs active. If @size is insufficient to contain * the return data -E2BIG is returned and @size is set to the minimum * required size. In all other cases a negative value indicating the * error is returned. */ SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, u32 __user *, size, u32, flags) { ... FTR: https://lwn.net/Articles/919059/
Created attachment 187550 [details] proposed patch
https://builder.sourceware.org/buildbot/#/changes/104990
(In reply to mcermak from comment #1) > Created attachment 187550 [details] > proposed patch Looks good with three small comments: > Declare lsm_list_modules wrappers in priv_syswrap-linux.h and hook it > for {amd64,arm,arm64,mips64,\ ppc32,ppc64,riscv64,s390x,x86}-linux. That \ in the middle is odd. + if (ML_(safe_to_deref)((__vki_u32 *)ARG2,sizeof(__vki_u32))) { + PRE_MEM_READ("lsm_list_modules(size)", ARG2, sizeof(__vki_u32)); + PRE_MEM_READ("lsm_list_modules(ids)", ARG1, *(__vki_u32 *)ARG2); + } The PRE_MEM_READ of the ARG2 value/pointer itself can be done without checking whether or not it can be dereferenced, so please do that before the if statement. The other PRE_MEM_READ does indeed need to be guarded because it dereferences ARG2. +POST(sys_lsm_list_modules) +{ + POST_MEM_WRITE((Addr)ARG2, sizeof(__vki_u32)); + POST_MEM_WRITE(ARG1, *(__vki_u32 *)ARG2); +} The first POST_MEM_WRITE is correct, the kernel will write out the actual size used. But also redundant because the value is (should) already (be) defined when going into the syscall. The value might change, but whether or not it is defined doesn't. Looks good to go with those issues addressed or explained.
(In reply to Mark Wielaard from comment #3) > +POST(sys_lsm_list_modules) > +{ > + POST_MEM_WRITE((Addr)ARG2, sizeof(__vki_u32)); > + POST_MEM_WRITE(ARG1, *(__vki_u32 *)ARG2); > +} > > The first POST_MEM_WRITE is correct, the kernel will write out the actual > size used. But also redundant because the value is (should) already (be) > defined when going into the syscall. The value might change, but whether or > not it is defined doesn't. Just to make sure I'm getting this right: Does the above mean that for memcheck's ARG2 memory bookkeeping purposes, the PRE_MEM_READ("lsm_list_modules(size)", ARG2, sizeof(__vki_u32)); is sufficient, and for that exact reason POST_MEM_WRITE((Addr)ARG2, sizeof(__vki_u32)); can be dropped? Doesn't memcheck need to track memory reads separately from memory writes?
(In reply to mcermak from comment #4) > (In reply to Mark Wielaard from comment #3) > > +POST(sys_lsm_list_modules) > > +{ > > + POST_MEM_WRITE((Addr)ARG2, sizeof(__vki_u32)); > > + POST_MEM_WRITE(ARG1, *(__vki_u32 *)ARG2); > > +} > > > > The first POST_MEM_WRITE is correct, the kernel will write out the actual > > size used. But also redundant because the value is (should) already (be) > > defined when going into the syscall. The value might change, but whether or > > not it is defined doesn't. > > Just to make sure I'm getting this right: Does the above mean that for > memcheck's ARG2 memory bookkeeping purposes, the > PRE_MEM_READ("lsm_list_modules(size)", ARG2, sizeof(__vki_u32)); is > sufficient, and for that exact reason POST_MEM_WRITE((Addr)ARG2, > sizeof(__vki_u32)); can be dropped? Doesn't memcheck need to track memory > reads separately from memory writes? Yes. What memcheck tracks is whether the address is accessible and whether the value at the address is defined. Technically the POST_MEM_WRITE isn't redundant. Whatever the value was (and whether it was defined or not) after the syscall it will have a defined value, because the kernel will have written to it (if it was addressable). But if that (new) value is derived from an undefined input value then I think we shouldn't really count it as properly defined. It doesn't really hurt though. The user will get a warning going into the syscall if the value at ARG2 isn't defined. And that might be enough. We can certainly pretend that afterwards the value is properly defined if the syscall succeeds (by accident).
Thank you for the clarification, noted. I've re-run the tests: https://builder.sourceware.org/buildbot/#/changes/105141
Pushed as commit ce7a2995b3194b4ad38a6f7f413988e259d98a24 .