linux-kernel - Re: [PATCH] selftests/exec: include cwd in long path calculation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8a8ae791-5c2c-a0b1-0f8f-83bad878b8f5@gmail.com>
Date:   Thu, 12 Oct 2017 10:36:55 -0700
From:   Steve Muckle <smuckle.linux@...il.com>
To:     David Drysdale <drysdale@...gle.com>
Cc:     Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] selftests/exec: include cwd in long path calculation

Thanks David for the review. Replies inline.

On 10/12/2017 05:22 AM, David Drysdale wrote:
> Modulo the minor comment below:
> 
> Reviewed-by: David Drysdale <drysdale@...gle.com>
> 
> On Thu, Oct 12, 2017 at 1:40 AM, Steve Muckle <smuckle.linux@...il.com> wrote:
>> When creating a pathname close to PATH_MAX to test execveat, factor in
>> the current working directory path otherwise we end up with an absolute
>> path that is longer than PATH_MAX. While execveat() may succeed, subsequent
>> calls to the kernel from the runtime environment which are required to
>> successfully execute the test binary/script may fail because of this.
>>
>> To keep the semantics of the test the same, rework the relative pathname
>> part of the test to be relative to the root directory so it isn't
>> decreased by the length of the current working directory path.
>>
>> Signed-off-by: Steve Muckle <smuckle.linux@...il.com>
>> ---
>>   tools/testing/selftests/exec/execveat.c | 24 +++++++++++++++++-------
>>   1 file changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/testing/selftests/exec/execveat.c b/tools/testing/selftests/exec/execveat.c
>> index 8d5d1d2ee7c1..5edc609c778b 100644
>> --- a/tools/testing/selftests/exec/execveat.c
>> +++ b/tools/testing/selftests/exec/execveat.c
>> @@ -147,7 +147,7 @@ static void exe_cp(const char *src, const char *dest)
>>   }
>>
>>   #define XX_DIR_LEN 200
>> -static int check_execveat_pathmax(int dot_dfd, const char *src, int is_script)
>> +static int check_execveat_pathmax(int root_dfd, const char *src, int is_script)
>>   {
>>          int fail = 0;
>>          int ii, count, len;
>> @@ -156,15 +156,24 @@ static int check_execveat_pathmax(int dot_dfd, const char *src, int is_script)
>>
>>          if (*longpath == '\0') {
>>                  /* Create a filename close to PATH_MAX in length */
>> +               char *cwd = getcwd(NULL, 0);
> 
> cwd never gets freed, but that's presumably not a problem unless these
> tests ever get run under some kind of leak checker.

Yeah I found some other instances of this in the test (specifically the 
calls to realpath() at the top of run_tests() which alloc their own 
buffer) and figured for a small unit test like this it had been decided 
it wasn't worth freeing stuff. I'll tidy this up anyway though.

> 
>> +
>> +               if (!cwd) {
>> +                       printf("Failed to getcwd(), errno=%d (%s)\n",
>> +                              errno, strerror(errno));
>> +                       return 2;
>> +               }
>> +               strcpy(longpath, cwd);
>> +               strcat(longpath, "/");
>>                  memset(longname, 'x', XX_DIR_LEN - 1);
>>                  longname[XX_DIR_LEN - 1] = '/';
>>                  longname[XX_DIR_LEN] = '\0';
>> -               count = (PATH_MAX - 3) / XX_DIR_LEN;
>> +               count = (PATH_MAX - 3 - strlen(cwd)) / XX_DIR_LEN;
>>                  for (ii = 0; ii < count; ii++) {
>>                          strcat(longpath, longname);
>>                          mkdir(longpath, 0755);
>>                  }
>> -               len = (PATH_MAX - 3) - (count * XX_DIR_LEN);
>> +               len = (PATH_MAX - 3 - strlen(cwd)) - (count * XX_DIR_LEN);
>>                  if (len <= 0)
>>                          len = 1;
>>                  memset(longname, 'y', len);
> 
> There's a missing comment update around here-ish:
>    * Execute as a long pathname relative to ".".
> (should now be 'relative to "/"' I think).

Will fix.

thanks,
Steve