[v2,4/4] Add error coverage for landlock network support

From: Andrea Cervesato <andrea.cervesato@suse.com>

Add two more errors checks inside the landlock02 which is testing
landlock_add_rule syscall. In particular, test now verifies when the
syscall is raising EINVAL due to invalid network attributes.

Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
---
 testcases/kernel/syscalls/landlock/landlock02.c | 94 +++++++++++++++++++------
 1 file changed, 74 insertions(+), 20 deletions(-)

Hi!
> diff --git a/testcases/kernel/syscalls/landlock/landlock02.c b/testcases/kernel/syscalls/landlock/landlock02.c
> index 8566d407f6d17ab367695125f07d0a80cf4130e5..dbc405a8a01ac58e0d22f952f57bd603c62ab8be 100644
> --- a/testcases/kernel/syscalls/landlock/landlock02.c
> +++ b/testcases/kernel/syscalls/landlock/landlock02.c
> @@ -20,93 +20,146 @@
>  
>  #include "landlock_common.h"
>  
> -static struct tst_landlock_ruleset_attr_abi1 *ruleset_attr;
> +static struct tst_landlock_ruleset_attr_abi4 *ruleset_attr;
>  static struct landlock_path_beneath_attr *path_beneath_attr;
>  static struct landlock_path_beneath_attr *rule_null;
> +static struct landlock_net_port_attr *net_port_attr;
>  static int ruleset_fd;
>  static int invalid_fd = -1;
> +static int abi_current;
>  
>  static struct tcase {
>  	int *fd;
> -	enum landlock_rule_type rule_type;
> -	struct landlock_path_beneath_attr **attr;
> +	int rule_type;
> +	struct landlock_path_beneath_attr **path_attr;
> +	struct landlock_net_port_attr **net_attr;
>  	int access;
>  	int parent_fd;
> +	int net_port;
>  	uint32_t flags;
>  	int exp_errno;
> +	int abi_ver;
>  	char *msg;
>  } tcases[] = {
>  	{
>  		.fd = &ruleset_fd,
> -		.attr = &path_beneath_attr,
> +		.path_attr = &path_beneath_attr,
> +		.net_attr = NULL,

This is a static structure, so anything that is not initialized will be
zeroed anyways, so I would just omit the explicit NULL initializations.

>  		.access = LANDLOCK_ACCESS_FS_EXECUTE,
>  		.flags = 1,
>  		.exp_errno = EINVAL,
> +		.abi_ver = 1,
>  		.msg = "Invalid flags"
>  	},
>  	{
>  		.fd = &ruleset_fd,
> -		.attr = &path_beneath_attr,
> +		.path_attr = &path_beneath_attr,
> +		.net_attr = NULL,
>  		.access = LANDLOCK_ACCESS_FS_EXECUTE,
>  		.exp_errno = EINVAL,
> +		.abi_ver = 1,
>  		.msg = "Invalid rule type"
>  	},
>  	{
>  		.fd = &ruleset_fd,
>  		.rule_type = LANDLOCK_RULE_PATH_BENEATH,
> -		.attr = &path_beneath_attr,
> +		.path_attr = &path_beneath_attr,
> +		.net_attr = NULL,
>  		.exp_errno = ENOMSG,
> +		.abi_ver = 1,
>  		.msg = "Empty accesses"
>  	},
>  	{
>  		.fd = &invalid_fd,
> -		.attr = &path_beneath_attr,
> +		.path_attr = &path_beneath_attr,
> +		.net_attr = NULL,
>  		.access = LANDLOCK_ACCESS_FS_EXECUTE,
>  		.exp_errno = EBADF,
> +		.abi_ver = 1,
>  		.msg = "Invalid file descriptor"
>  	},
>  	{
>  		.fd = &ruleset_fd,
>  		.rule_type = LANDLOCK_RULE_PATH_BENEATH,
> -		.attr = &path_beneath_attr,
> +		.path_attr = &path_beneath_attr,
> +		.net_attr = NULL,
>  		.access = LANDLOCK_ACCESS_FS_EXECUTE,
>  		.parent_fd = -1,
>  		.exp_errno = EBADF,
> +		.abi_ver = 1,
>  		.msg = "Invalid parent fd"
>  	},
>  	{
>  		.fd = &ruleset_fd,
>  		.rule_type = LANDLOCK_RULE_PATH_BENEATH,
> -		.attr = &rule_null,
> +		.path_attr = &rule_null,
> +		.net_attr = NULL,
>  		.exp_errno = EFAULT,
> +		.abi_ver = 1,
>  		.msg = "Invalid rule attr"
>  	},
> +	{
> +		.fd = &ruleset_fd,
> +		.rule_type = LANDLOCK_RULE_NET_PORT,
> +		.path_attr = NULL,
> +		.net_attr = &net_port_attr,
> +		.access = LANDLOCK_ACCESS_FS_EXECUTE,
> +		.net_port = 448,
> +		.exp_errno = EINVAL,
> +		.abi_ver = 4,
> +		.msg = "Invalid access rule for network type"
> +	},
> +	{
> +		.fd = &ruleset_fd,
> +		.rule_type = LANDLOCK_RULE_NET_PORT,
> +		.path_attr = NULL,
> +		.net_attr = &net_port_attr,
> +		.access = LANDLOCK_ACCESS_NET_BIND_TCP,
> +		.net_port = INT16_MAX + 1,
> +		.exp_errno = EINVAL,
> +		.abi_ver = 4,
> +		.msg = "Socket port greater than 65535"
> +	},
>  };
>  
>  static void run(unsigned int n)
>  {
>  	struct tcase *tc = &tcases[n];
>  
> -	if (*tc->attr) {
> -		(*tc->attr)->allowed_access = tc->access;
> -		(*tc->attr)->parent_fd = tc->parent_fd;
> +	if (tc->abi_ver > abi_current) {
> +		tst_res(TCONF, "Minimum ABI required: %d", tc->abi_ver);
> +		return;
>  	}
>  
> -	TST_EXP_FAIL(tst_syscall(__NR_landlock_add_rule,
> -		*tc->fd, tc->rule_type, *tc->attr, tc->flags),
> -		tc->exp_errno,
> -		"%s",
> -		tc->msg);
> +	if (tc->path_attr) {
> +		if (*tc->path_attr) {
> +			(*tc->path_attr)->allowed_access = tc->access;
> +			(*tc->path_attr)->parent_fd = tc->parent_fd;
> +		}
> +
> +		TST_EXP_FAIL(tst_syscall(__NR_landlock_add_rule,
> +			*tc->fd, tc->rule_type, *tc->path_attr, tc->flags),
> +			tc->exp_errno, "%s", tc->msg);
> +	} else if (tc->net_attr) {
> +		if (*tc->net_attr) {
> +			(*tc->net_attr)->allowed_access = tc->access;
> +			(*tc->net_attr)->port = tc->net_port;
> +		}
> +
> +		TST_EXP_FAIL(tst_syscall(__NR_landlock_add_rule,
> +			*tc->fd, tc->rule_type, *tc->net_attr, tc->flags),
> +			tc->exp_errno, "%s", tc->msg);

if we assing the attr into a pointer this TST_EPX_FAIL() can be outside
of the if as:

	void *attr;

	if (path_attr) {
		...
		attr = *path_attr;
	} else {
		...
		attr = *net_attr;
	}

	TST_EXP_FAIL(..., attr, ...);

> +	}
>  }
>  
>  static void setup(void)
>  {
> -	verify_landlock_is_enabled();
> +	abi_current = verify_landlock_is_enabled();
>  
>  	ruleset_attr->handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE;
>  
>  	ruleset_fd = TST_EXP_FD_SILENT(tst_syscall(__NR_landlock_create_ruleset,
> -		ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi1), 0));
> +		ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi4), 0));
                               ^
			       This should be abi_current otherwise we
			       will fail on v1 only system.

>  }
>  
>  static void cleanup(void)
> @@ -122,8 +175,9 @@ static struct tst_test test = {
>  	.cleanup = cleanup,
>  	.needs_root = 1,
>  	.bufs = (struct tst_buffers []) {
> -		{&ruleset_attr, .size = sizeof(struct tst_landlock_ruleset_attr_abi1)},
> +		{&ruleset_attr, .size = sizeof(struct tst_landlock_ruleset_attr_abi4)},
>  		{&path_beneath_attr, .size = sizeof(struct landlock_path_beneath_attr)},
> +		{&net_port_attr, .size = sizeof(struct landlock_net_port_attr)},
>  		{},
>  	},
>  	.caps = (struct tst_cap []) {

The rest looks good to me, with the minor probles fixed:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

Hi!
> >  	ruleset_fd = TST_EXP_FD_SILENT(tst_syscall(__NR_landlock_create_ruleset,
> > -		ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi1), 0));
> > +		ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi4), 0));
>                                ^
> 			       This should be abi_current otherwise we
> 			       will fail on v1 only system.
                                 ^
                              Not exactly right either.

We have to pass:

MIN(abi, sizeof(struct tst_landlock_ruleset_attr_abi4))

To make sure that we enable either v1 or v4.

Hi Cyril,

On 11/5/24 16:39, Cyril Hrubis wrote:
>> -	verify_landlock_is_enabled();
>> +	abi_current = verify_landlock_is_enabled();
>>   
>>   	ruleset_attr->handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE;
>>   
>>   	ruleset_fd = TST_EXP_FD_SILENT(tst_syscall(__NR_landlock_create_ruleset,
>> -		ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi1), 0));
>> +		ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi4), 0));
>                                 ^
> 			       This should be abi_current otherwise we
> 			       will fail on v1 only system.
>
>>   }
>>   

In what sense? abi4 is already the last one. At least the last supported 
by LTP.

Andrea

Hi!
> >> -	verify_landlock_is_enabled();
> >> +	abi_current = verify_landlock_is_enabled();
> >>   
> >>   	ruleset_attr->handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE;
> >>   
> >>   	ruleset_fd = TST_EXP_FD_SILENT(tst_syscall(__NR_landlock_create_ruleset,
> >> -		ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi1), 0));
> >> +		ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi4), 0));
> >                                 ^
> > 			       This should be abi_current otherwise we
> > 			       will fail on v1 only system.
> >
> >>   }
> >>   
> 
> In what sense? abi4 is already the last one. At least the last supported 
> by LTP.

Because if we request abi4 it will fail on kernels that only support
abi1. We try hard to skip the abi4 tests, but we wouldn't get there at
all on abi1 kernel because we would fail to create the ruleset_fd in the
test setup.

And we cannot initialize the abi to anything newer than abi4 either,
because we pass abi4 structure in the test. It's fine that we pass abi4
structure on abi1 system here, because the test only checks for invalid
cases and all we need here is to pass a valid attr and size so that we
get rejected by the kernel on the rest of the parameters.

On 11/6/24 11:38, Cyril Hrubis wrote:
> Hi!
>>>> -	verify_landlock_is_enabled();
>>>> +	abi_current = verify_landlock_is_enabled();
>>>>    
>>>>    	ruleset_attr->handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE;
>>>>    
>>>>    	ruleset_fd = TST_EXP_FD_SILENT(tst_syscall(__NR_landlock_create_ruleset,
>>>> -		ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi1), 0));
>>>> +		ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi4), 0));
>>>                                  ^
>>> 			       This should be abi_current otherwise we
>>> 			       will fail on v1 only system.
>>>
>>>>    }
>>>>    
>> In what sense? abi4 is already the last one. At least the last supported
>> by LTP.
> Because if we request abi4 it will fail on kernels that only support
> abi1. We try hard to skip the abi4 tests, but we wouldn't get there at
> all on abi1 kernel because we would fail to create the ruleset_fd in the
> test setup.
>
> And we cannot initialize the abi to anything newer than abi4 either,
> because we pass abi4 structure in the test. It's fine that we pass abi4
> structure on abi1 system here, because the test only checks for invalid
> cases and all we need here is to pass a valid attr and size so that we
> get rejected by the kernel on the rest of the parameters.
>
I can instantiate both ABI1 and ABI4 structs, then I pass it to the 
tcase accordingly.

Andrea