Re: [LTP] [PATCH 2/2] setregid02, setresuid03: inval_user fix

[Stanislav K. <sta...@or...>]

On 01/13/2014 09:38 PM, ch...@su... wrote:
> Hi!
>> diff --git a/testcases/kernel/syscalls/setresuid/setresuid03.c b/testcases/kernel/syscalls/setresuid/setresuid03.c
>> index 9bb0abd..085ed7b 100644
>> --- a/testcases/kernel/syscalls/setresuid/setresuid03.c
>> +++ b/testcases/kernel/syscalls/setresuid/setresuid03.c
>> @@ -70,10 +70,12 @@
>>   #include <errno.h>
>>   #include <sys/wait.h>
>>
>> +#include <compat_16.h>
>> +
>>   char *TCID = "setresuid03";
>>
>>   uid_t neg_one = -1;
>> -uid_t inval_user = (USHRT_MAX - 2);
>> +uid_t inval_user;
>>
>>   /* flag to tell parent if child passed or failed. */
>>   int flag = 0;
>> @@ -232,6 +234,10 @@ void setup(void)
>>   	bin = *(getpwnam("bin"));
>>   	bin_pw_uid = bin.pw_uid;
>>
>> +	inval_user = GET_UNUSED_UID();
>> +	if (inval_user == -1)
>> +		tst_brkm(TBROK, NULL, "No free uid found");
>> +
>>   	/* Pause if that option was specified
>>   	 * TEST_PAUSE contains the code to fork the test with the -i option.
>>   	 * You want to make sure you do this before you create your temporary
>
> Hmm, the setresuid03 did not fail because the EPERM check kicks in even
> before the check for correct uid/gid. Which should probably be fixed
> because there does not seem to be a setresuid test for invalid values
> that would actually excercise such checks, but that can be fixed after
> the release.

Hmm. I took a brief look at setregid() and setresuid() syscalls sources.
And it seems that the only thing they care about is not "validity" or 
"invalidity" of uid/gid. It's the process permission (capabilities).

Given enough permission we can change real, effective or saved uid 
values with setresuid() to arbitrary values. If we don't have such 
permission we can change our uids only to the one of the current real, 
effective or saved uid values, in the other case we will receive EPERM. 
Actually, man setresuid states that :-[

setresuid() may return EINVAL, but I don't think we can raise it by 
passing an unused uid value to setresuid() (if we have permission 
setresuid() returns 0 if we don't - it returns EPERM).

So it's not bad that we are introducing tst_get_unused_uid, 
tst_get_unused_gid functions and setting inval_user in a more formal way.
But It looks like passing inval_user value is the same as passing
any other uid != real|effective|saved uid of the current process....

Since we have in setresuid03 a full set for nobody uid:
	setresuid(nobody, -1, -1)
	setresuid(-1, -1, nobody)
	setresuid(-1, nobody, -1)
what do you think about adding setresuid(inval_user, -1, -1) to the test 
case? Just in case...

Related to setregid02.
Even that test_data contains EINVAL for the last two members, setregid() 
doesn't return EINVAL for them. It returns EPERM for all the test cases.

You said that on SUSE setregid02 fails. I added a group with 65533 gid 
and executed setregid02 (current git version) and it passed. Could you 
check it again in your environment?

>
> Thanks for the patches, I will commit them right after packages are
> build and I have tested that everything is fine.
>