[LTP] [PATCH] syscalls/semctl03: Solve kernel panic in semctl03

[Dylan Jhong]

On Fri, Aug 26, 2022 at 03:53:22PM +0800, Richard Palethorpe wrote:
> Hello,
> 
> Dylan Jhong <dylan@andestech.com> writes:
> 
> > Hi Richard,
> >
> > Thanks for your reply.
> > My opinion is the same as yours, libc should do more checking and
> > protection for incoming parameters
> 
> This is not my opinion.
> 
> Are you saying that libc segfaults? This is an acceptable outcome for
> the LTP. To stop the test failing we can fork the test and check if the
> child segfaults. However it seems the EFAULT test is already skipped if
> we use libc, which is also acceptable.
> 
> However the patch title says that this resulted in a kernel panic due to
> a null pointer dereference? This is a serious kernel bug that may be
> exploitable.
> 

>>>>> Are you saying that libc segfaults? This is an acceptable outcome for the LTP. To stop the test failing we can fork the test and check if the child segfaults. However it seems the EFAULT test is already skipped if we use libc, which is also acceptable.

It's segmentation fault from glibc. Sorry for the confusion.
If there is a V2 version, I will modify the title.

The failure case comes from the code below,
which expect EINVAL as the return value.

tests[] = {
	{&sem_id, -1, &semds_ptr, EINVAL, "invalid IPC command"},
	{&bad_id, IPC_STAT, &semds_ptr, EINVAL, "invalid sem id"},  <-- Segfault occurs on this testcase
	{&sem_id, GETALL, &bad_ptr, EFAULT, "invalid union arg"},
	{&sem_id, IPC_SET, &bad_ptr, EFAULT, "invalid union arg"}
};

This is correct in some architechures. But on other architectures where 
__IPC_TIME64 is defined, this segmentation fault will occur in glibc.

When those architectures that define __IPC_TIME64 call semctl(), glibc will 
additionally enter a conversion function named semun64_to_ksemun64()[*1].
Then the 4th parameter, "semun64.buf" from semctl() will be passed to the 
next function[*2]. Finally a segmentation fault occurs in the 
semid64_to_ksemid64() function[*3].

The purpose of this test case should be to detect if glibc returns EINVAL 
when we pass bad_id to semctl(), but not every architecture can get this
result. The segmentation fault caused by semun64.buf is NULL is obviously 
not the expected result of this testcase, so I think it should be the
correct way to modify the 4th argument pass to semctl().

[*1] https://github.com/bminor/glibc/blob/f94f6d8a3572840d3ba42ab9ace3ea522c99c0c2/sysdeps/unix/sysv/linux/semctl.c#L172
[*2] https://github.com/bminor/glibc/blob/f94f6d8a3572840d3ba42ab9ace3ea522c99c0c2/sysdeps/unix/sysv/linux/semctl.c#L107
[*3] https://github.com/bminor/glibc/blob/f94f6d8a3572840d3ba42ab9ace3ea522c99c0c2/sysdeps/unix/sysv/linux/semctl.c#L68

Best regards,
Dylan Jhong

> >
> > In semctl03.c, the two tv->semctl() implementation functions, which are libc_semctl() and sys_semctl(),
> > do not pass the 4th argument ".buf" to the next level system call.
> > At present, the 4th argument of semctl() implemented in semctl03.c is hard-coded,
> > I think passing parameters instead of hardcoding should be more better for this testcase.
> > Should we pass all parameters to the next level semctl() system call?
> 
> A 4th arg is never passed, if you remove the vararg the test compiles
> and runs fine. So the vararg should be removed, but this is relatively
> minor compared to a kernel null pointer dereference.
> 
> -- 
> Thank you,
> Richard.