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

[Dylan Jhong <dylan@andestech.com>]

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