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

[Richard Palethorpe]

Hello,

Dylan Jhong <dylan@andestech.com> writes:

> 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().

Thanks, this clears up the confusion, I'll modify the description and merge.

>
> [*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.


-- 
Thank you,
Richard.