[RFC] debug: Add tests for fortified fcntl ()

  Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 debug/tst-fortify.c | 158 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 158 insertions(+)
  

I think it would be better to include this test on the fcntl fortify patch
itself, or at least on a patchset so the first one is set as prerequisite.

On 20/05/23 15:21, Sergey Bugaev via Libc-alpha wrote:
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  debug/tst-fortify.c | 158 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 158 insertions(+)
> 
> diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c
> index 7850a4e5..17a15de7 100644
> --- a/debug/tst-fortify.c
> +++ b/debug/tst-fortify.c
> @@ -78,6 +78,15 @@ do_prepare (void)
>      }
>  }
>  
> +/* Return VALUE, but do it in a way that the compiler cannot
> +   see that it's a compile-time constant.  */
> +static int __attribute_noinline__
> +hide_constant (int value)
> +{
> +  volatile int v = value;
> +  return v;
> +}
> +
>  volatile int chk_fail_ok;
>  volatile int ret;
>  jmp_buf chk_fail_buf;
> @@ -1763,6 +1772,155 @@ do_test (void)
>    ppoll (fds, l0 + 2, NULL, NULL);
>    CHK_FAIL_END
>  # endif
> +#endif
> +
> +  /* Check that we can do basic fcntl operations, both ones that require
> +     the third argument, and ones that do not.  */
> +  res = fcntl (STDIN_FILENO, F_GETFD);
> +  TEST_COMPARE (res, 0);
> +  res = fcntl (STDIN_FILENO, F_SETFD, 0);
> +  TEST_COMPARE (res, 0);
> +
> +#ifdef F_OFD_GETLK
> +  /* Check for confusion between 32- and 64-bit versions of the fcntl
> +     interface.  But first, check that the kernel supports OFD locks at all,
> +     using a non-fortified function.  */
> +  int lockfd1 = xopen (temp_filename, O_RDWR, 0);
> +  int lockfd2 = xopen (temp_filename, O_RDWR, 0);

It misses the inclusion of 'support/xunistd.h>' (the testcase fails for build
it).

> +
> +  struct flock flock;
> +  memset (&flock, 0, sizeof (flock));
> +  flock.l_type = F_WRLCK;
> +  flock.l_whence = SEEK_SET;
> +  flock.l_start = 0;
> +  flock.l_len = INT32_MAX;
> +  flock.l_pid = 0;
> +
> +  res = __fcntl (lockfd1, F_OFD_GETLK, &flock);

This is an external case, so use the external default name.

> +  int ofd_locks_supported = (res != -1 || errno != EINVAL);
> +
> +  if (ofd_locks_supported)
> +    {
> +      TEST_COMPARE (res, 0);
> +      TEST_COMPARE (flock.l_type, F_UNLCK);
> +
> +      memset (&flock, 0, sizeof (flock));
> +      flock.l_type = F_WRLCK;
> +      flock.l_whence = SEEK_SET;
> +      flock.l_start = 1234;
> +      flock.l_len = 5678;
> +      flock.l_pid = 0;
> +
> +      res = fcntl (lockfd1, F_OFD_SETLK, &flock);
> +      TEST_COMPARE (res, 0);
> +
> +      memset (&flock, 0, sizeof (flock));
> +      flock.l_type = F_RDLCK;
> +      flock.l_whence = SEEK_SET;
> +      flock.l_start = 3542;
> +      flock.l_len = 411;
> +      flock.l_pid = 0;
> +
> +      res = fcntl (lockfd2, F_OFD_GETLK, &flock);
> +      TEST_COMPARE (res, 0);
> +      /* Check that we get the expected values.  */
> +      TEST_COMPARE (flock.l_type, F_WRLCK);
> +      TEST_COMPARE (flock.l_whence, SEEK_SET);
> +      TEST_COMPARE (flock.l_start, 1234);
> +      TEST_COMPARE (flock.l_len, 5678);
> +      TEST_COMPARE (flock.l_pid, -1);
> +    }
> +#endif
> +
> +  /* Check that we can do fcntl operations with CMD that is not constant
> +     at compile time.  */
> +  res = fcntl (STDIN_FILENO, hide_constant (F_GETFD));
> +  TEST_COMPARE (res, 0);
> +  res = fcntl (STDIN_FILENO, hide_constant (F_SETFD), 0);
> +  TEST_COMPARE (res, 0);
> +
> +#ifdef F_OFD_GETLK
> +  if (ofd_locks_supported)
> +    {
> +      memset (&flock, 0, sizeof (flock));
> +      flock.l_type = F_RDLCK;
> +      flock.l_whence = SEEK_SET;
> +      flock.l_start = 3542;
> +      flock.l_len = 411;
> +      flock.l_pid = 0;
> +
> +      res = fcntl (lockfd2, hide_constant (F_OFD_GETLK), &flock);
> +      TEST_COMPARE (res, 0);
> +      /* Check that we get the expected values.  */
> +      TEST_COMPARE (flock.l_type, F_WRLCK);
> +      TEST_COMPARE (flock.l_whence, SEEK_SET);
> +      TEST_COMPARE (flock.l_start, 1234);
> +      TEST_COMPARE (flock.l_len, 5678);
> +      TEST_COMPARE (flock.l_pid, -1);
> +    }
> +#endif
> +
> +#if __USE_FORTIFY_LEVEL >= 1
> +  CHK_FAIL_START
> +  fcntl (STDIN_FILENO, hide_constant (F_SETFD));
> +  CHK_FAIL_END
> +#endif
> +
> +#if defined (__USE_LARGEFILE64) || defined (__USE_TIME_BITS64)
> +  /* Also check fcntl64 ().  */
> +  res = fcntl64 (STDIN_FILENO, F_GETFD);
> +  TEST_COMPARE (res, 0);
> +  res = fcntl64 (STDIN_FILENO, F_SETFD, 0);
> +  TEST_COMPARE (res, 0);
> +  res = fcntl64 (STDIN_FILENO, hide_constant (F_GETFD));
> +  TEST_COMPARE (res, 0);
> +  res = fcntl64 (STDIN_FILENO, hide_constant (F_SETFD), 0);
> +  TEST_COMPARE (res, 0);
> +
> +#ifdef F_OFD_GETLK
> +  if (ofd_locks_supported)
> +    {
> +      struct flock64 flock64;
> +
> +      memset (&flock64, 0, sizeof (flock64));
> +      flock64.l_type = F_RDLCK;
> +      flock64.l_whence = SEEK_SET;
> +      flock64.l_start = 3542;
> +      flock64.l_len = 411;
> +      flock64.l_pid = 0;
> +
> +      res = fcntl64 (lockfd2, F_OFD_GETLK, &flock64);
> +      TEST_COMPARE (res, 0);
> +      /* Check that we get the expected values.  */
> +      TEST_COMPARE (flock64.l_type, F_WRLCK);
> +      TEST_COMPARE (flock64.l_whence, SEEK_SET);
> +      TEST_COMPARE (flock64.l_start, 1234);
> +      TEST_COMPARE (flock64.l_len, 5678);
> +      TEST_COMPARE (flock64.l_pid, -1);
> +
> +      memset (&flock64, 0, sizeof (flock64));
> +      flock64.l_type = F_RDLCK;
> +      flock64.l_whence = SEEK_SET;
> +      flock64.l_start = 3542;
> +      flock64.l_len = 411;
> +      flock64.l_pid = 0;
> +
> +      res = fcntl64 (lockfd2, hide_constant (F_OFD_GETLK), &flock64);
> +      TEST_COMPARE (res, 0);
> +      /* Check that we get the expected values.  */
> +      TEST_COMPARE (flock64.l_type, F_WRLCK);
> +      TEST_COMPARE (flock64.l_whence, SEEK_SET);
> +      TEST_COMPARE (flock64.l_start, 1234);
> +      TEST_COMPARE (flock64.l_len, 5678);
> +      TEST_COMPARE (flock64.l_pid, -1);
> +    }
> +#endif
> +
> +# if __USE_FORTIFY_LEVEL >= 1
> +  CHK_FAIL_START
> +  fcntl64 (STDIN_FILENO, hide_constant (F_SETFD));
> +  CHK_FAIL_END
> +# endif
>  #endif
>  
>    return ret;
  

Hello,

thank you for taking a look.

On Tue, May 23, 2023 at 9:40 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
> > +  res = __fcntl (lockfd1, F_OFD_GETLK, &flock);
>
> This is an external case, so use the external default name.

Do you mean that this should be s/__fcntl/fcntl/?

If that is what you mean -- here's why I used __fcntl here, but not in
all the other places:

The point of the OFD tests in the patch is to check for 32-bit vs
64-bit confusion in the fortification. If, for example, in some
configuration the plain fcntl is supposed to be 32-bit (and struct
flock is 32-bit), but the fortified version of fcntl mistakenly
forwards to fcntl64, then the 32-bit struct flock will not be
converted to struct flock64, and will be passed to the kernel as-is.
It will yield some garbage when interpreted as a struct flock64, and
the kernel will likely reject it with an error (EINVAL, perhaps) --
which is what the test checks against.

But the test also needs to work on the kernel versions that do not
support OFD locks, so it does this check for support being present at
all first, and skips running the OFD tests if the kernel does not
support it. But how do you check for OFD locks support? -- you try to
F_OFD_GETLK and see if the kernel returns EINVAL. If it does return
that, OFD locks are unsupported.

And herein lies the problem, we cannot differentiate between EINVALs
caused by missing kernel support from EINVALs caused by the very kind
of bugs this test checks against! We'd mistake the actual issue we
wanted to catch for missing kernel support, skip the bulk of the test,
and report success.

So in this first check of whether or not OFD locks are supported at
all, I've used __fcntl, which does not get fortified and so cannot
suffer from the 32-bit vs 64-bit confusion bug. The rest of the code
uses the regular fcntl. There's a comment above briefly mentioning
this, too -- clearly it has to be expanded.

So, with this in mind, should I be using __fcntl here, or is there a
better option?

Sergey
  

On 23/05/23 16:19, Sergey Bugaev wrote:
> Hello,
> 
> thank you for taking a look.
> 
> On Tue, May 23, 2023 at 9:40 PM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>> +  res = __fcntl (lockfd1, F_OFD_GETLK, &flock);
>>
>> This is an external case, so use the external default name.
> 
> Do you mean that this should be s/__fcntl/fcntl/?

The __fnctl is not provided in all build configurations, so debug tests
fail to build:

debug/tst-fortify-cc-default-1-def.o:

./tst-fortify.c:1800:9: error: ‘__fcntl’ was not declared in this scope; did you mean ‘fcntl’?
 1800 |   res = __fcntl (lockfd1, F_OFD_GETLK, &flock);
      |         ^~~~~~~
      |         fcntl

> 
> If that is what you mean -- here's why I used __fcntl here, but not in
> all the other places:
> 
> The point of the OFD tests in the patch is to check for 32-bit vs
> 64-bit confusion in the fortification. If, for example, in some
> configuration the plain fcntl is supposed to be 32-bit (and struct
> flock is 32-bit), but the fortified version of fcntl mistakenly
> forwards to fcntl64, then the 32-bit struct flock will not be
> converted to struct flock64, and will be passed to the kernel as-is.
> It will yield some garbage when interpreted as a struct flock64, and
> the kernel will likely reject it with an error (EINVAL, perhaps) --
> which is what the test checks against.

But we already have different tests for non-LFS and LFS on debug,
so any issue will be reported a test failure (including wrong 
fortification redirection).

> 
> But the test also needs to work on the kernel versions that do not
> support OFD locks, so it does this check for support being present at
> all first, and skips running the OFD tests if the kernel does not
> support it. But how do you check for OFD locks support? -- you try to
> F_OFD_GETLK and see if the kernel returns EINVAL. If it does return
> that, OFD locks are unsupported.
> 
> And herein lies the problem, we cannot differentiate between EINVALs
> caused by missing kernel support from EINVALs caused by the very kind
> of bugs this test checks against! We'd mistake the actual issue we
> wanted to catch for missing kernel support, skip the bulk of the test,
> and report success.
> 
> So in this first check of whether or not OFD locks are supported at
> all, I've used __fcntl, which does not get fortified and so cannot
> suffer from the 32-bit vs 64-bit confusion bug. The rest of the code
> uses the regular fcntl. There's a comment above briefly mentioning
> this, too -- clearly it has to be expanded.
> 
> So, with this in mind, should I be using __fcntl here, or is there a
> better option?

I think a better option would to move the kernel probe support to libsupport
and call it instead.
  

Hello,

On Tue, May 23, 2023 at 10:49 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
> The __fnctl is not provided in all build configurations, so debug tests
> fail to build:
>
> debug/tst-fortify-cc-default-1-def.o:
>
> ./tst-fortify.c:1800:9: error: ‘__fcntl’ was not declared in this scope; did you mean ‘fcntl’?
>  1800 |   res = __fcntl (lockfd1, F_OFD_GETLK, &flock);
>       |         ^~~~~~~
>       |         fcntl

I see, thanks :|

> But we already have different tests for non-LFS and LFS on debug,
> so any issue will be reported a test failure (including wrong
> fortification redirection).

Could you please point out which tests you're talking about? I've only
seen tst-fortify get built/checked in all the various configurations
(_FORTIFY_SOURCE or not,  _GNU_SOURCE or not, _FILE_OFFSET_BITS=64 or
not, _LARGEFILE64_SOURCE or not).

> > So, with this in mind, should I be using __fcntl here, or is there a
> > better option?
>
> I think a better option would to move the kernel probe support to libsupport
> and call it instead.

That makes a lot of sense, will do, thanks.

Sergey
  

On 24/05/23 04:15, Sergey Bugaev wrote:
> Hello,
> 
> On Tue, May 23, 2023 at 10:49 PM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>> The __fnctl is not provided in all build configurations, so debug tests
>> fail to build:
>>
>> debug/tst-fortify-cc-default-1-def.o:
>>
>> ./tst-fortify.c:1800:9: error: ‘__fcntl’ was not declared in this scope; did you mean ‘fcntl’?
>>  1800 |   res = __fcntl (lockfd1, F_OFD_GETLK, &flock);
>>       |         ^~~~~~~
>>       |         fcntl
> 
> I see, thanks :|
> 
>> But we already have different tests for non-LFS and LFS on debug,
>> so any issue will be reported a test failure (including wrong
>> fortification redirection).
> 
> Could you please point out which tests you're talking about? I've only
> seen tst-fortify get built/checked in all the various configurations
> (_FORTIFY_SOURCE or not,  _GNU_SOURCE or not, _FILE_OFFSET_BITS=64 or
> not, _LARGEFILE64_SOURCE or not).

For ABI with non native 64 bit time_t (i686-linux-gnu for instance) we also
have _TIME_BITS=64.

But I think the issues of wrong redirection by the fortify itself is not easily
caught without dumping the binary symbol table and check which function is
actually generated by compiler.

> 
>>> So, with this in mind, should I be using __fcntl here, or is there a
>>> better option?
>>
>> I think a better option would to move the kernel probe support to libsupport
>> and call it instead.
> 
> That makes a lot of sense, will do, thanks.
> 
> Sergey