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

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

[Dylan Jhong]

When using semctl() through glibc and __IPC_TIME64 is defined, glibc will
call a converted semun64_to_ksemun64() function[*1]. If the parameter of
this function is NULL, it will cause a NULL pointer dereference kernel
panic.

In semctl03.c, we need to ensure the element "struct semid_ds *buf" in 4th
parameter "union semun" in semctl() is not NULL. But the 4th parameters of
libc_semctl() and sys_semctl() are hard-coded[*2] and the element
"struct semid_ds *buf" is not given an initial value. Using va_list to pass
the correct parameters can solve the problem.

ref:
  [*1]: https://github.com/bminor/glibc/blob/f94f6d8a3572840d3ba42ab9ace3ea522c99c0c2/sysdeps/unix/sysv/linux/semctl.c#L172
  [*2]: https://github.com/linux-test-project/ltp/blob/58caa8cca507133ea92bd0ea277b91add96e72af/testcases/kernel/syscalls/ipc/semctl/semctl03.c#L45

Co-developed-by: Randolph <randolph@andestech.com>
Signed-off-by: Dylan Jhong <dylan@andestech.com>
---
 testcases/kernel/syscalls/ipc/semctl/semctl03.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/testcases/kernel/syscalls/ipc/semctl/semctl03.c b/testcases/kernel/syscalls/ipc/semctl/semctl03.c
index a1a4c81ce..bb25053e2 100644
--- a/testcases/kernel/syscalls/ipc/semctl/semctl03.c
+++ b/testcases/kernel/syscalls/ipc/semctl/semctl03.c
@@ -28,11 +28,21 @@ static union semun arg = {0};
 
 static int libc_semctl(int semid, int semnum, int cmd, ...)
 {
+	va_list ap;
+
+	va_start(ap, cmd);
+	arg = va_arg(ap, union semun);
+	va_end(ap);
 	return semctl(semid, semnum, cmd, arg);
 }
 
 static int sys_semctl(int semid, int semnum, int cmd, ...)
 {
+	va_list ap;
+
+	va_start(ap, cmd);
+	arg = va_arg(ap, union semun);
+	va_end(ap);
 	return tst_syscall(__NR_semctl, semid, semnum, cmd, arg);
 }
 
-- 
2.34.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

[Richard Palethorpe]

Hello,

Dylan Jhong <dylan@andestech.com> writes:

> When using semctl() through glibc and __IPC_TIME64 is defined, glibc will
> call a converted semun64_to_ksemun64() function[*1]. If the parameter of
> this function is NULL, it will cause a NULL pointer dereference kernel
> panic.

This is a kernel bug. Generally speaking, we shouldn't be able to create
kernel panics from user land. The kernel should return EFAULT if we pass
an invalid pointer.

If this test causes a kernel panic then it should be kept as-is. If it
is not testing what it was originally intended to, then another test can
be created to do that.

>
> In semctl03.c, we need to ensure the element "struct semid_ds *buf" in 4th
> parameter "union semun" in semctl() is not NULL. But the 4th parameters of
> libc_semctl() and sys_semctl() are hard-coded[*2] and the element
> "struct semid_ds *buf" is not given an initial value. Using va_list to pass
> the correct parameters can solve the problem.
>
> ref:
>   [*1]: https://github.com/bminor/glibc/blob/f94f6d8a3572840d3ba42ab9ace3ea522c99c0c2/sysdeps/unix/sysv/linux/semctl.c#L172
>   [*2]: https://github.com/linux-test-project/ltp/blob/58caa8cca507133ea92bd0ea277b91add96e72af/testcases/kernel/syscalls/ipc/semctl/semctl03.c#L45
>
> Co-developed-by: Randolph <randolph@andestech.com>
> Signed-off-by: Dylan Jhong <dylan@andestech.com>
> ---
>  testcases/kernel/syscalls/ipc/semctl/semctl03.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/testcases/kernel/syscalls/ipc/semctl/semctl03.c b/testcases/kernel/syscalls/ipc/semctl/semctl03.c
> index a1a4c81ce..bb25053e2 100644
> --- a/testcases/kernel/syscalls/ipc/semctl/semctl03.c
> +++ b/testcases/kernel/syscalls/ipc/semctl/semctl03.c
> @@ -28,11 +28,21 @@ static union semun arg = {0};
>  
>  static int libc_semctl(int semid, int semnum, int cmd, ...)
>  {
> +	va_list ap;
> +
> +	va_start(ap, cmd);
> +	arg = va_arg(ap, union semun);
> +	va_end(ap);
>  	return semctl(semid, semnum, cmd, arg);
>  }
>  
>  static int sys_semctl(int semid, int semnum, int cmd, ...)
>  {
> +	va_list ap;
> +
> +	va_start(ap, cmd);
> +	arg = va_arg(ap, union semun);
> +	va_end(ap);
>  	return tst_syscall(__NR_semctl, semid, semnum, cmd, arg);
>  }
>  
> -- 
> 2.34.1


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

[Dylan Jhong]

Hi Richard,

Thanks for your reply.
My opinion is the same as yours, libc should do more checking and protection for incoming parameters

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?

Partial code of semctl03.c:
--------------------------------------------------------
TST_EXP_FAIL(tv->semctl(*(tc->sem_id), 0, tc->ipc_cmd, *(tc->buf)),    <--- Pass *(tc->buf) to tv->semctl()
        tc->error, "semctl() with %s", tc->message);


static union semun arg = {0};
static int libc_semctl(int semid, int semnum, int cmd, ...)  
{
    return semctl(semid, semnum, cmd, arg);        <----- Ignore the 4th parameter and use the hard-coded "arg" directly  
}
--------------------------------------------------------

ref:
    https://lists.linux.it/pipermail/ltp/2021-June/023116.html

Best,
Dylan

On Fri, Aug 26, 2022 at 02:12:19PM +0800, Richard Palethorpe wrote:
> Hello,
> 
> Dylan Jhong <dylan@andestech.com> writes:
> 
> > When using semctl() through glibc and __IPC_TIME64 is defined, glibc will
> > call a converted semun64_to_ksemun64() function[*1]. If the parameter of
> > this function is NULL, it will cause a NULL pointer dereference kernel
> > panic.
> 
> This is a kernel bug. Generally speaking, we shouldn't be able to create
> kernel panics from user land. The kernel should return EFAULT if we pass
> an invalid pointer.
> 
> If this test causes a kernel panic then it should be kept as-is. If it
> is not testing what it was originally intended to, then another test can
> be created to do that.
> 
> >
> > In semctl03.c, we need to ensure the element "struct semid_ds *buf" in 4th
> > parameter "union semun" in semctl() is not NULL. But the 4th parameters of
> > libc_semctl() and sys_semctl() are hard-coded[*2] and the element
> > "struct semid_ds *buf" is not given an initial value. Using va_list to pass
> > the correct parameters can solve the problem.
> >
> > ref:
> >   [*1]: https://github.com/bminor/glibc/blob/f94f6d8a3572840d3ba42ab9ace3ea522c99c0c2/sysdeps/unix/sysv/linux/semctl.c#L172
> >   [*2]: https://github.com/linux-test-project/ltp/blob/58caa8cca507133ea92bd0ea277b91add96e72af/testcases/kernel/syscalls/ipc/semctl/semctl03.c#L45
> >
> > Co-developed-by: Randolph <randolph@andestech.com>
> > Signed-off-by: Dylan Jhong <dylan@andestech.com>
> > ---
> >  testcases/kernel/syscalls/ipc/semctl/semctl03.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/testcases/kernel/syscalls/ipc/semctl/semctl03.c b/testcases/kernel/syscalls/ipc/semctl/semctl03.c
> > index a1a4c81ce..bb25053e2 100644
> > --- a/testcases/kernel/syscalls/ipc/semctl/semctl03.c
> > +++ b/testcases/kernel/syscalls/ipc/semctl/semctl03.c
> > @@ -28,11 +28,21 @@ static union semun arg = {0};
> >  
> >  static int libc_semctl(int semid, int semnum, int cmd, ...)
> >  {
> > +	va_list ap;
> > +
> > +	va_start(ap, cmd);
> > +	arg = va_arg(ap, union semun);
> > +	va_end(ap);
> >  	return semctl(semid, semnum, cmd, arg);
> >  }
> >  
> >  static int sys_semctl(int semid, int semnum, int cmd, ...)
> >  {
> > +	va_list ap;
> > +
> > +	va_start(ap, cmd);
> > +	arg = va_arg(ap, union semun);
> > +	va_end(ap);
> >  	return tst_syscall(__NR_semctl, semid, semnum, cmd, arg);
> >  }
> >  
> > -- 
> > 2.34.1
> 
> 
> -- 
> Thank you,
> Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

[Richard Palethorpe]

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.

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [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.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [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.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp