[LTP] [PATCH v4] adjtimex02.c: Skipped EFAULT tests for libc variant"

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> Tested EFAULT cases only for "__NR_adjtimex" syscall.
> 
> Tests for bad addresses in LTP cases trigger segment
> fault in libc on a 32bit system.
> 
> Signed-off-by: Vinay Kumar <vinay.m.engg@gmail.com>
> ---
>  .../kernel/syscalls/adjtimex/adjtimex02.c     | 226 ++++++++++++------
>  1 file changed, 152 insertions(+), 74 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/adjtimex/adjtimex02.c b/testcases/kernel/syscalls/adjtimex/adjtimex02.c
> index 19ee97158..5eaea6352 100644
> --- a/testcases/kernel/syscalls/adjtimex/adjtimex02.c
> +++ b/testcases/kernel/syscalls/adjtimex/adjtimex02.c
> @@ -10,115 +10,193 @@
>  #include <unistd.h>
>  #include <pwd.h>
>  #include "tst_test.h"
> +#include "lapi/syscalls.h"
>  
> -#define SET_MODE ( ADJ_OFFSET | ADJ_FREQUENCY | ADJ_MAXERROR | ADJ_ESTERROR | \
> -	ADJ_STATUS | ADJ_TIMECONST | ADJ_TICK )
> +#define SET_MODE (ADJ_OFFSET | ADJ_FREQUENCY | ADJ_MAXERROR | ADJ_ESTERROR | \
> +				ADJ_STATUS | ADJ_TIMECONST | ADJ_TICK)
>  
> -static int hz;			/* HZ from sysconf */
> -
> -static struct timex *tim_save;
> -static struct timex *buf;
> +static int hz;		/* HZ from sysconf */
>  
> +static struct timex *tim_save, *buf;
>  static struct passwd *ltpuser;
>  
> -static void verify_adjtimex(unsigned int nr)
> +struct test_case {
> +	unsigned int modes;
> +	long lowlimit;
> +	long highlimit;
> +	long delta;
> +	int exp_err;
> +};
> +
> +static int libc_adjtimex(void *timex)
>  {
> -	struct timex *bufp;
> -	int expected_errno = 0;
> +	return adjtimex(timex);
> +}

Do we need this indirection?

As long as we fix the prototype in the test_variants to have a proper
type for the function, i.e. the timex pointer is struct timex * instead
of void * we can store the libc function pointer directly to the variant
structure.

> -	/*
> -	 * since Linux 2.6.26, if buf.offset value is outside
> -	 * the acceptable range, it is simply normalized instead
> -	 * of letting the syscall fail. so just skip this test
> -	 * case.
> -	 */
> -	if (nr > 3 && (tst_kvercmp(2, 6, 25) > 0)) {
> -		tst_res(TCONF, "this kernel normalizes buf."
> -				"offset value if it is outside"
> -				" the acceptable range.");
> -		return;
> -	}
> +static int sys_adjtimex(void *timex)
                            ^
			    This should be struct timex *
> +{
> +	return tst_syscall(__NR_adjtimex, timex);
> +}
> +
> +static struct test_case tc[] = {
> +	{
> +	.modes = SET_MODE,
> +	.exp_err = EFAULT,
> +	},
> +	{
> +	.modes = ADJ_TICK,
> +	.lowlimit = 900000,
> +	.delta = 1,
> +	.exp_err = EINVAL,
> +	},
> +	{
> +	.modes = ADJ_TICK,
> +	.highlimit = 1100000,
> +	.delta = 1,
> +	.exp_err = EINVAL,
> +	},
> +	{
> +	.modes = ADJ_OFFSET,
> +	.highlimit = 512000L,
> +	.delta = 1,
> +	.exp_err = EINVAL,
> +	},
> +	{
> +	.modes = ADJ_OFFSET,
> +	.lowlimit = -512000L,
> +	.delta = -1,
> +	.exp_err = EINVAL,
> +	},
> +};
> +
> +static struct test_variants
> +{
> +	int (*adjtimex)(void *timex);
                        ^
			This should be struct timex * as well.
> +	char *desc;
> +} variants[] = {
> +	{ .adjtimex = libc_adjtimex, .desc = "libc adjtimex()"},
> +
> +#if (__NR_adjtimex != __LTP__NR_INVALID_SYSCALL)
> +	{ .adjtimex = sys_adjtimex,  .desc = "__NR_adjtimex syscall"},
> +#endif
> +};
> +
> +static void verify_adjtimex(unsigned int i)
> +{
> +	struct timex *bufp;
> +	struct test_variants *tv = &variants[tst_variant];
>  
>  	*buf = *tim_save;
>  	buf->modes = SET_MODE;
>  	bufp = buf;
> -	switch (nr) {
> -	case 0:
> -		bufp = (struct timex *)-1;
> -		expected_errno = EFAULT;
> -		break;
> -	case 1:
> -		buf->tick = 900000 / hz - 1;
> -		expected_errno = EINVAL;
> -		break;
> -	case 2:
> -		buf->tick = 1100000 / hz + 1;
> -		expected_errno = EINVAL;
> -		break;
> -	case 3:
> -		/* Switch to nobody user for correct error code collection */
> -		ltpuser = SAFE_GETPWNAM("nobody");
> -		SAFE_SETEUID(ltpuser->pw_uid);
> -		expected_errno = EPERM;
> -		break;
> -	case 4:
> -		buf->offset = 512000L + 1;
> -		expected_errno = EINVAL;
> -		break;
> -	case 5:
> -		buf->offset = (-1) * (512000L) - 1;
> -		expected_errno = EINVAL;
> -		break;
> -	default:
> -		tst_brk(TFAIL, "Invalid test case %u ", nr);
> +
> +	if (tc[i].exp_err == EINVAL) {
> +		if (tc[i].modes == ADJ_TICK) {
> +			if (tc[i].lowlimit)
> +				buf->tick = tc[i].lowlimit - tc[i].delta;
> +
> +			if (tc[i].highlimit)
> +				buf->tick = tc[i].highlimit + tc[i].delta;
> +		}
> +		if (tc[i].modes == ADJ_OFFSET) {
> +			if (tc[i].lowlimit) {
> +				tst_res(TCONF, "this kernel normalizes buf. offset value if it is outside the acceptable range.");
> +				return;
> +			}
> +			if (tc[i].highlimit) {
> +				tst_res(TCONF, "this kernel normalizes buf. offset value if it is outside the acceptable range.");
> +				return;
> +			}
> +		}
>  	}
>  
> -	TEST(adjtimex(bufp));
> -	if ((TST_RET == -1) && (TST_ERR == expected_errno)) {
> -		tst_res(TPASS | TTERRNO,
> -				"adjtimex() error %u ", expected_errno);
> -	} else {
> -		tst_res(TFAIL | TTERRNO,
> -				"Test Failed, adjtimex() returned %ld",
> -				TST_RET);
> +	if (tc[i].exp_err == EFAULT) {
> +		if (tv->adjtimex != libc_adjtimex) {
> +			bufp = (struct timex *) -1;
> +		} else {
> +			tst_res(TCONF, "EFAULT is skipped for libc variant");
> +			return;
> +		}
> +	}
> +
> +	TEST(tv->adjtimex(bufp));
> +
> +	if (tc[i].exp_err != TST_ERR) {
> +		tst_res(TFAIL | TTERRNO, "adjtimex(): expected %s mode %x",
> +					tst_strerrno(tc[i].exp_err), tc[i].modes);
> +		return;
>  	}

We should really use TST_EXP_FAIL() here instead.

> -	/* clean up after ourselves */
> -	if (nr == 3)
> -		SAFE_SETEUID(0);
> +	tst_res(TPASS, "adjtimex() error %s", tst_strerrno(tc[i].exp_err));
> +
>  }
>  
>  static void setup(void)
>  {
> +	struct test_variants *tv = &variants[tst_variant];
> +	size_t i;
> +	int expected_errno = 0;
> +
> +	tst_res(TINFO, "Testing variant: %s", tv->desc);
> +
>  	tim_save->modes = 0;
>  
> +	buf->modes = SET_MODE;
> +
> +	/* Switch to nobody user for correct error code collection */
> +	ltpuser = SAFE_GETPWNAM("nobody");
> +	SAFE_SETEUID(ltpuser->pw_uid);
> +	expected_errno = EPERM;
> +
> +	TEST(tv->adjtimex(buf));
> +
> +	if ((TST_RET == -1) && (TST_ERR == expected_errno)) {
> +		tst_res(TPASS, "adjtimex() error %s ",
> +				tst_strerrno(expected_errno));
> +	} else {
> +		tst_res(TFAIL | TTERRNO,
> +				"adjtimex(): returned %ld", TST_RET);
> +	}
> +
> +	SAFE_SETEUID(0);

What is this nonsense?

What is doing a test code in the test setup?

The only thing that should be done in the setup is to store the nobody
uid into a global variable.

The EPERM test should be done in the verify_adjtimex() function, we can
do something as:

	if (tc[i].exp_err == EPERM)
		SAFE_SETEUID(nobody_uid);


	/* do the actuall test */

	if (tc[i].exp_err == EPERM)
		SAFE_SETEUID(0);

>  	/* set the HZ from sysconf */
>  	hz = SAFE_SYSCONF(_SC_CLK_TCK);
>  
> -	/* Save current parameters */
> -	if ((adjtimex(tim_save)) == -1)
> +	for (i = 0; i < ARRAY_SIZE(tc); i++) {
> +		if (tc[i].modes == ADJ_TICK) {
> +			tc[i].highlimit /= hz;
> +			tc[i].lowlimit /= hz;
> +		}
> +	}
> +
> +	if ((adjtimex(tim_save)) == -1) {
>  		tst_brk(TBROK | TERRNO,
> -			"adjtimex(): failed to save current params");
> +		"adjtimex(): failed to save current params");
> +	}
>  }
>  
>  static void cleanup(void)
>  {
> +
>  	tim_save->modes = SET_MODE;
>  
> -	/* Restore saved parameters */
> -	if ((adjtimex(tim_save)) == -1)
> -		tst_res(TWARN, "Failed to restore saved parameters");
> +	if ((adjtimex(tim_save)) == -1) {
> +		tst_brk(TBROK | TERRNO,
> +		"adjtimex(): failed to save current params");
                                  ^
				  This was correct before the change, we
				  change the modes to SET_MODE so we
				  really restore the value here.
> +	}
>  }
>  
>  static struct tst_test test = {
> -	.needs_root = 1,
> -	.tcnt = 6,
> +	.test = verify_adjtimex,
>  	.setup = setup,
>  	.cleanup = cleanup,
> -	.test = verify_adjtimex,
> +	.tcnt = ARRAY_SIZE(tc),
> +	.test_variants = ARRAY_SIZE(variants),
> +	.needs_root = 1,
>  	.bufs = (struct tst_buffers []) {
> -		{&buf, .size = sizeof(*buf)},
> -		{&tim_save, .size = sizeof(*tim_save)},
> -		{},
> -	}
> +		 {&buf, .size = sizeof(*buf)},
> +		 {&tim_save, .size = sizeof(*tim_save)},
> +		 {},
> +		}
>  };
> -- 
> 2.17.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz