| Message ID | 20230730192605.2423480-2-bugaevc@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | fcntl fortification | expand |
On 30/07/23 16:25, Sergey Bugaev via Libc-alpha wrote: > F_GETLK, F_SETLK, F_SETLKW must always be used with the regular struct > flock, not struct flock64, even if the function being called is fcntl64. > There are separate commands, F_GETLK64, F_SETLK64, and F_SETLKW64, that > are used with struct flock64. This is in contrast with the OFD locking > commands (F_OFD_GETLK, F_OFD_SETLK, F_OFD_SETLKW) that must be used with > struct flock in fcntl calls, and with struct flock64 in fcntl64 calls. > > These mistakes were spotted by enabling the fcntl fortification that is > added in a following commit. > > Fixes: 61d3db428176d9d0822e4e680305fe34285edff2 > "login: pututxline could fail to overwrite existing entries [BZ #24902]" > Fixes: f6233ab412c3bebebacf65745e775e01506dd58d > "Linux: Add io/tst-o_path-locks test" > > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> Nice, it seems that on both tests the fields that might trigger wrong values by the kernel reading the struct with a wrong size (l_start and l_len) are always being 0 and thus it should not matter. LGTM, thanks. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > --- > > Please tell me if I'm misunderstanding this! > > login/tst-pututxline-lockfail.c | 2 +- > sysdeps/unix/sysv/linux/tst-o_path-locks.c | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c > index 214d1106..ca82ba42 100644 > --- a/login/tst-pututxline-lockfail.c > +++ b/login/tst-pututxline-lockfail.c > @@ -77,7 +77,7 @@ subprocess_lock_file (void) > .l_type = F_RDLCK, > fl.l_whence = SEEK_SET, > }; > - TEST_COMPARE (fcntl64 (fd, F_SETLKW, &fl), 0); > + TEST_COMPARE (fcntl64 (fd, F_SETLKW64, &fl), 0); > > /* Signal to the main process that the lock has been acquired. */ > xpthread_barrier_wait (barrier); > diff --git a/sysdeps/unix/sysv/linux/tst-o_path-locks.c b/sysdeps/unix/sysv/linux/tst-o_path-locks.c > index 0036868d..c3d1c0dc 100644 > --- a/sysdeps/unix/sysv/linux/tst-o_path-locks.c > +++ b/sysdeps/unix/sysv/linux/tst-o_path-locks.c > @@ -39,7 +39,7 @@ subprocess (void *closure) > { > int fd = xopen (path, O_RDWR, 0); > struct flock64 lock = { .l_type = F_WRLCK, }; > - int ret = fcntl64 (fd, F_SETLK, &lock); > + int ret = fcntl64 (fd, F_SETLK64, &lock); > if (ret == 0) > *shared_errno = 0; > else > @@ -76,7 +76,7 @@ do_test (void) > TEST_VERIFY (!probe_lock ()); > > struct flock64 lock = { .l_type = F_WRLCK, }; > - TEST_COMPARE (fcntl64 (fd, F_SETLK, &lock), 0); > + TEST_COMPARE (fcntl64 (fd, F_SETLK64, &lock), 0); > > /* The lock has been acquired. */ > TEST_VERIFY (probe_lock ()); > @@ -87,7 +87,7 @@ do_test (void) > TEST_VERIFY (!probe_lock ()); > > /* But not if it is an O_PATH descriptor. */ > - TEST_COMPARE (fcntl64 (fd, F_SETLK, &lock), 0); > + TEST_COMPARE (fcntl64 (fd, F_SETLK64, &lock), 0); > xclose (xopen (path, O_PATH, 0)); > TEST_VERIFY (probe_lock ()); >
Hello, On Mon, Jul 31, 2023 at 8:50 PM Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote: > Nice, it seems that on both tests the fields that might trigger wrong values > by the kernel reading the struct with a wrong size (l_start and l_len) are > always being 0 and thus it should not matter. > > LGTM, thanks. But please note that this only really fortifies tests & other code that uses the public fcntl/fcntl64 API, not callers of __libc_fcntl64, so it's not a guarantee that there are no more cases of this. Though I guess this applies equally to all the other fortifications. Sergey
diff --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c index 214d1106..ca82ba42 100644 --- a/login/tst-pututxline-lockfail.c +++ b/login/tst-pututxline-lockfail.c @@ -77,7 +77,7 @@ subprocess_lock_file (void) .l_type = F_RDLCK, fl.l_whence = SEEK_SET, }; - TEST_COMPARE (fcntl64 (fd, F_SETLKW, &fl), 0); + TEST_COMPARE (fcntl64 (fd, F_SETLKW64, &fl), 0); /* Signal to the main process that the lock has been acquired. */ xpthread_barrier_wait (barrier); diff --git a/sysdeps/unix/sysv/linux/tst-o_path-locks.c b/sysdeps/unix/sysv/linux/tst-o_path-locks.c index 0036868d..c3d1c0dc 100644 --- a/sysdeps/unix/sysv/linux/tst-o_path-locks.c +++ b/sysdeps/unix/sysv/linux/tst-o_path-locks.c @@ -39,7 +39,7 @@ subprocess (void *closure) { int fd = xopen (path, O_RDWR, 0); struct flock64 lock = { .l_type = F_WRLCK, }; - int ret = fcntl64 (fd, F_SETLK, &lock); + int ret = fcntl64 (fd, F_SETLK64, &lock); if (ret == 0) *shared_errno = 0; else @@ -76,7 +76,7 @@ do_test (void) TEST_VERIFY (!probe_lock ()); struct flock64 lock = { .l_type = F_WRLCK, }; - TEST_COMPARE (fcntl64 (fd, F_SETLK, &lock), 0); + TEST_COMPARE (fcntl64 (fd, F_SETLK64, &lock), 0); /* The lock has been acquired. */ TEST_VERIFY (probe_lock ()); @@ -87,7 +87,7 @@ do_test (void) TEST_VERIFY (!probe_lock ()); /* But not if it is an O_PATH descriptor. */ - TEST_COMPARE (fcntl64 (fd, F_SETLK, &lock), 0); + TEST_COMPARE (fcntl64 (fd, F_SETLK64, &lock), 0); xclose (xopen (path, O_PATH, 0)); TEST_VERIFY (probe_lock ());
F_GETLK, F_SETLK, F_SETLKW must always be used with the regular struct flock, not struct flock64, even if the function being called is fcntl64. There are separate commands, F_GETLK64, F_SETLK64, and F_SETLKW64, that are used with struct flock64. This is in contrast with the OFD locking commands (F_OFD_GETLK, F_OFD_SETLK, F_OFD_SETLKW) that must be used with struct flock in fcntl calls, and with struct flock64 in fcntl64 calls. These mistakes were spotted by enabling the fcntl fortification that is added in a following commit. Fixes: 61d3db428176d9d0822e4e680305fe34285edff2 "login: pututxline could fail to overwrite existing entries [BZ #24902]" Fixes: f6233ab412c3bebebacf65745e775e01506dd58d "Linux: Add io/tst-o_path-locks test" Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> --- Please tell me if I'm misunderstanding this! login/tst-pututxline-lockfail.c | 2 +- sysdeps/unix/sysv/linux/tst-o_path-locks.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)