On 18/08/2022 14:44, Kevin Brodsky wrote:
On 17/08/2022 17:47, Zachary Leaf wrote:
On success, shmat syscall returns a void* to the address of the
I'd remove "the address of", otherwise it sounds like a pointer to a pointer :)
attached memory segment. Enable shmat to return a capability
... in PCuABI ...
Ack.
by annotating it as returning a ptr.
At this stage, derive and return a root user capability, without any capability checks or logic.
Signed-off-by: Zachary Leaf zachary.leaf@arm.com
review branch at: https://git.morello-project.org/zdleaf/linux/-/tree/fork/review/shmat
relevant LTP tests: shmat01, shmat02, kill05, kill07
include/uapi/asm-generic/unistd.h | 2 +- ipc/shm.c | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h index 736ca77b8101..be084b38187d 100644 --- a/include/uapi/asm-generic/unistd.h +++ b/include/uapi/asm-generic/unistd.h @@ -590,7 +590,7 @@ __SYSCALL(__NR_shmget, sys_shmget) #define __NR_shmctl 195 __SC_COMP(__NR_shmctl, sys_shmctl, compat_sys_shmctl) #define __NR_shmat 196 -__SC_COMP(__NR_shmat, sys_shmat, compat_sys_shmat) +__SC_COMP_RETPTR(__NR_shmat, sys_shmat, compat_sys_shmat) #define __NR_shmdt 197 __SYSCALL(__NR_shmdt, sys_shmdt) diff --git a/ipc/shm.c b/ipc/shm.c index 9485f6474146..f5805f5c8d42 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -1678,7 +1678,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, return err; } -SYSCALL_DEFINE3(shmat, int, shmid, char __user *, shmaddr, int, shmflg) +SYSCALL_DEFINE3(__retptr__(shmat), int, shmid, char __user *, shmaddr, int, shmflg) { unsigned long ret; long err; @@ -1686,8 +1686,12 @@ SYSCALL_DEFINE3(shmat, int, shmid, char __user *, shmaddr, int, shmflg) err = do_shmat(shmid, shmaddr, shmflg, &ret, SHMLBA); if (err) return err; + if (IS_ERR_VALUE(ret)) + return ret;
Unless I'm missing something, do_shmat() never gives you an invalid pointer if it returns 0? See the few lines after the call to do_mmap() in particular.
Correct. Looking closer, this is not required and is covered by if (err) above. Will remove in v2.
force_successful_syscall_return(); - return (long)ret; + /* TODO [PCuABI] - derive proper capability */ + return (user_uintptr_t)uaddr_to_user_ptr_safe((ptraddr_t)ret);
No need to cast ret as it's an unsigned long (i.e. the same type as ptraddr_t in practice).
Got it.
Thanks, Zach
Kevin
} #ifdef CONFIG_COMPAT