From: Florian Weimer <fweimer@redhat.com>
To: Sergey Bugaev via Libc-alpha <libc-alpha@sourceware.org>
Cc: Sergey Bugaev <bugaevc@gmail.com>
Subject: Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments
Date: Wed, 24 May 2023 10:29:09 +0200 [thread overview]
Message-ID: <87fs7myvxm.fsf@oldenburg.str.redhat.com> (raw)
In-Reply-To: <CAN9u=HfgLaRZfmHvA1-G2Vytm_Kwr5PuXma3zRDri6ZSBjFBzw@mail.gmail.com> (Sergey Bugaev via Libc-alpha's message of "Wed, 24 May 2023 10:31:26 +0300")
[-- Attachment #1: Type: text/plain, Size: 5078 bytes --]
* Sergey Bugaev via Libc-alpha:
> Hello,
>
> On Wed, May 24, 2023 at 12:46 AM Florian Weimer <fw@deneb.enyo.de> wrote:
>> A while ago, I looked into implementing this with
>> __builtin_types_compatible_p and __builtin_choose_expr. It seemed
>> feasible (although I didn't complete it), but C++ would require a
>> completely different implementation.
>>
>> I'm not sure if this goes in the right direction. Maybe we should add
>> specialized functions for the common fcntl requests. This way, we'd
>> get compile-time type checking in a far more maintainable manner.
>
> Interesting -- I have thought of doing type checking, but haven't
> found a way to make it work.
>
> There doesn't seem to be a way to extract an argument value / type out
> of a __builtin_va_arg_pack. Even if you know that
> __builtin_va_arg_pack_len () == 1, you cannot do:
>
> int arg = __builtin_va_arg_pack ();
>
> or
>
> int __fcntl3_int (int fd, int cmd, int arg);
> __fcntl3_int (fd, cmd, __builtin_va_arg_pack ());
>
> In other words: __builtin_va_arg_pack () does not behave like a macro
> that gets textually expanded to the anonymous argument(s), but really
> as a value of the "..." "argument".
In my attempt, the top level looks like this:
+#define fcntl(fd, cmd, ...) \
+ (__builtin_constant_p (cmd) \
+ ? __builtin_choose_expression \
+ (__fcntl_is_void (cmd), __fcntl_void (fd, cmd, __VA_ARGS__), \
+ __builtin_choose_expression \
+ (__fcntl_is_int (cmd), __fcntl_int (fd, cmd, __VA_ARGS__), \
+ __builtin_choose_expression \
+ (__fcntl_is_flock_const (cmd), __fcntl_flock_const (fd, cmd, __VA_ARGS__), \
+ __builtin_choose_expression \
+ (__fcntl_is_flock (cmd), __fcntl_flock (fd, cmd, __VA_ARGS__), \
+ __builtin_chose_expression \
+ (__fcntl_is_flock64_const (cmd), __fcntl_flock64_const (fd, cmd, __VA_ARGS__), \
+ __builtin_choose_expression \
+ (__fcntl_is_flock64 (cmd), __fcntl_flock64 (fd, cmd, __VA_ARGS__), \
+ __builtin_choose_expression \
+ (__fcntl_is_flock32_const (cmd), __fcntl_flock32_const (fd, cmd, __VA_ARGS__), \
+ __builtin_choose_expression \
+ (__fcntl_is_flock32 (cmd), __fcntl_flock32 (fd, cmd, __VA_ARGS__), \
+ __fcntl_unchecked (fd, cmd, __VA_ARGS__))))))))) \
+ : __fcntl_unchecked (fd, cmd, __VA_ARGS__))
IIRC, it doesn't quite work because __builtin_choose_expression only
suppresses errors, but not warnings, in the branch that wasn't chosen. 8-(
Maybe this is something that could be fixed with _Generic, using
__builtin_choose_expression for the __fcntl_is_void check only.
The type predicts look like this:
+#ifdef F_SETLK64
+# define __fcntl_is_flock64_const(cmd) \
+ ((cmd) == F_SETLK64 || (cmd) == F_SETLKW64)
+# define __fcntl_is_flock64(cmd) ((cmd) == F_GETLK64)
+#else
+# define __fcntl_is_flock64_const(cmd) 0
+# define __fcntl_is_flock64(cmd) 0
+#endif
The helper wrappers are simple redirects to the existing exported
function, with transparent unions to handle the aliases.
+#if defined (__USE_LARGEFILE64) && defined (__OFF_T_MATCHES_OFF64_T)
+typedef union __attribute__ ((__transparent_union__))
+{
+ struct flock *__flock;
+ struct flock64 *__flock64;
+} __flock_pointer;
+typedef union __attribute__ ((__transparent_union__))
+{
+ const struct flock *__flock;
+ const struct flock64 *__flock64;
+} __flock_const_pointer;
+#else /* flock and flock64 are distinct */
+typedef struct flock *__flock_pointer;
+typedef const struct flock *__flock_const_pointer;
+#endif
+
+int __REDIRECT (__fcntl_flock, (int, int, __flock_pointer), __fcntl_chk) __wur;
+int __REDIRECT (__fcntl_flock_const, (int, int, __flock_const_pointer),
+ __fcntl_chk) __wur;
+int __REDIRECT (__fcntl_flock32, (int, int, struct flock *),
+ __fcntl_chk) __wur;
+int __REDIRECT (__fcntl_flock32_const, (int, int, const struct flock *),
+ __fcntl_chk) __wur;
+#ifdef __USE_LARGEFILE64
+int __REDIRECT (__fcntl_flock64, (int, int, struct flock64 *),
+ __fcntl_chk) __wur;
+int __REDIRECT (__fcntl_flock64_const, (int, int, const struct flock64 *),
+ __fcntl_chk) __wur;
+#endif
The compiler will then warn about the type mismatches
(-Wincompatible-pointer-types is not an error by default, for backwards
compatibility). But as I said, I don't think this approach worked
because __builtin_choose_expression does not suppress those warnings.
I'm attaching my broken patch. It's based on commit ef4f97648dc9584
(from 2016).
Thanks,
Florian
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fcntl.patch --]
[-- Type: text/x-patch, Size: 8786 bytes --]
commit badf4c4724dcd838d27b40f92eb7d0039a968793
Author: Florian Weimer <fweimer@redhat.com>
Date: Thu Sep 1 15:49:45 2016 +0200
WIP fcntl hardening
diff --git a/include/bits/fcntl3.h b/include/bits/fcntl3.h
new file mode 100644
index 0000000000..d31154f635
--- /dev/null
+++ b/include/bits/fcntl3.h
@@ -0,0 +1 @@
+#include "../../io/bits/fcntl3.h"
diff --git a/io/Makefile b/io/Makefile
index deb6100156..98603e012c 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -25,7 +25,7 @@ include ../Makeconfig
headers := sys/stat.h bits/stat.h sys/statfs.h bits/statfs.h sys/vfs.h \
sys/statvfs.h bits/statvfs.h fcntl.h sys/fcntl.h bits/fcntl.h \
poll.h sys/poll.h bits/poll.h bits/fcntl2.h bits/poll2.h \
- utime.h ftw.h fts.h sys/sendfile.h
+ utime.h ftw.h fts.h sys/sendfile.h bits/fcntl3.h
routines := \
utime \
diff --git a/io/bits/fcntl3.h b/io/bits/fcntl3.h
new file mode 100644
index 0000000000..f9778ecec3
--- /dev/null
+++ b/io/bits/fcntl3.h
@@ -0,0 +1,180 @@
+/* Checking macros for fcntl functions.
+ Copyright (C) 2016 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef _FCNTL_H
+# error "Never include <bits/fcntl3.h> directly; use <fcntl.h> instead."
+#endif
+
+/* The type-safe aliases call the __fcntl_chk function. Only calls
+ which are not known to be type-safe use fcntl. */
+int __fcntl_chk (int, int, ...);
+
+__errordecl (__fcntl_too_many_args,
+ "fcntl can only be called with 2 or 3 arguments");
+
+/* Void arguments are ignored if present, and the return value must be
+ used. */
+__fortify_function __wur int
+__fcntl_void (int __fd, int __cmd, ...)
+{
+ if (__va_arg_pack_len () > 1)
+ __fcntl_too_many_args ();
+ return __fcntl_chk (__fd, __cmd, __va_arg_pack ());
+}
+
+#ifdef F_GET_SEALS
+# define __fcntl_is_F_GET_SEALS(cmd) ((cmd) == F_GET_SEALS)
+#else
+# define __fcntl_is_F_GET_SEALS(cmd) 0
+#endif
+#define __fcntl_is_void(cmd) \
+ ((cmd) == F_GET_FD \
+ || (cmd) == F_GETFL \
+ || (cmd) == F_GETOWN \
+ || (cmd) == F_GETSIG \
+ || (cmd) == __fcntl_is_F_GET_SEALS(cmd))
+
+int __REDIRECT (__fcntl_int, (int, int, int __arg), __fcntl_chk);
+#ifdef F_ADD_SEALS
+# define __fcntl_is_F_ADD_SEALS(cmd) (cmd) == F_ADD_SEALS
+#else
+# define __fcntl_is_F_ADD_SEALS(cmd) 0
+#endif
+#define __fcntl_is_int(cmd) \
+ ((cmd) == F_DUPFD \
+ || (cmd) == F_DUPFD_CLOEXEC \
+ || (cmd) == F_SETFD \
+ || (cmd) == F_SETFL \
+ || (cmd) == F_SETOWN \
+ || (cmd) == F_SETSIG \
+ || (cmd) == F_SETLEASE \
+ || (cmd) == F_NOTIFY \
+ || (cmd) == F_SETPIPE_SZ \
+ || __fcntl_is_F_ADD_SEALS (cmd))
+
+int __REDIRECT (__fcntl_f_owner_ex, (int, int, struct f_owner_ex *),
+ __fcntl_chk);
+int __REDIRECT (__fcntl_f_owner_ex_const,
+ (int, int, const struct f_owner_ex *),
+ __fcntl_chk);
+
+/* struct flock *, struct flock64 * arguments. */
+#define __fcntl_is_flock_const(cmd) ((cmd) == F_SETLK || (cmd) == F_SETLKW)
+#define __fcntl_is_flock(cmd) ((cmd) == F_GETLK)
+
+#ifdef F_SETLK64
+# define __fcntl_is_flock64_const(cmd) \
+ ((cmd) == F_SETLK64 || (cmd) == F_SETLKW64)
+# define __fcntl_is_flock64(cmd) ((cmd) == F_GETLK64)
+#else
+# define __fcntl_is_flock64_const(cmd) 0
+# define __fcntl_is_flock64(cmd) 0
+#endif
+#ifdef F_SETLK32
+# define __fcntl_is_flock32_const(cmd) \
+ ((cmd) == F_SETLK32 || (cmd) == F_SETLKW32)
+# define __fcntl_is_flock32(cmd) ((cmd) == F_GETLK32)
+#else
+# define __fcntl_is_flock32_const(cmd) 0
+# define __fcntl_is_flock32(cmd) 0
+#endif
+
+#if defined (__USE_LARGEFILE64) && defined (__OFF_T_MATCHES_OFF64_T)
+typedef union __attribute__ ((__transparent_union__))
+{
+ struct flock *__flock;
+ struct flock64 *__flock64;
+} __flock_pointer;
+typedef union __attribute__ ((__transparent_union__))
+{
+ const struct flock *__flock;
+ const struct flock64 *__flock64;
+} __flock_const_pointer;
+#else /* flock and flock64 are distinct */
+typedef struct flock *__flock_pointer;
+typedef const struct flock *__flock_const_pointer;
+#endif
+
+int __REDIRECT (__fcntl_flock, (int, int, __flock_pointer), __fcntl_chk) __wur;
+int __REDIRECT (__fcntl_flock_const, (int, int, __flock_const_pointer),
+ __fcntl_chk) __wur;
+int __REDIRECT (__fcntl_flock32, (int, int, struct flock *),
+ __fcntl_chk) __wur;
+int __REDIRECT (__fcntl_flock32_const, (int, int, const struct flock *),
+ __fcntl_chk) __wur;
+#ifdef __USE_LARGEFILE64
+int __REDIRECT (__fcntl_flock64, (int, int, struct flock64 *),
+ __fcntl_chk) __wur;
+int __REDIRECT (__fcntl_flock64_const, (int, int, const struct flock64 *),
+ __fcntl_chk) __wur;
+#endif
+
+#ifdef F_OFD_SETLK
+# define __fcntl_is_ofd_flock_const(cmd) \
+ ((cmd) == F_OFD_SETLK || (cmd) == F_OFD_SETLKW)
+# define __fcntl_is_ofd_flock(cmd) ((cmd) == F_OFD_GETLK)
+
+/* OFD locks are only available with 64-bit struct flock. */
+# if defined (__OFF_T_MATCHES_OFF64_T)
+int __REDIRECT (__fcntl_ofd_flock, (int, int, __flock_pointer),
+ __fcntl_chk) __wur;
+int __REDIRECT (__fcntl_ofd_flock_const, (int, int, __flock_const_pointer),
+ __fcntl_chk) __wur;
+# else
+int __REDIRECT (__fcntl_ofd_flock, (int, int, struct flock64 *),
+ __fcntl_chk) __wur;
+int __REDIRECT (__fcntl_ofd_flock_const, (int, const struct flock64 *),
+ __fcntl_chk) __wur;
+# endif
+
+#else /* !defined (F_OFD_SETLK) */
+# define __fcntl_is_ofd_flock_const (cmd) 0
+# define __fcntl_is_ofd_flock (cmd) 0
+#endif
+
+extern int __REDIRECT (__fcntl_warn, (int, int, ...), fcntl)
+ __warnattr ("fcntl called with an unknown command argument");
+
+__fortify_function int
+__fcntl_unchecked (int __fd, int __cmd, ...)
+{
+ if (__va_arg_pack_len () > 1)
+ __fcntl_too_many_args ();
+ return __fcntl_warn (__fd, __cmd, __va_arg_pack ());
+}
+
+#define fcntl(fd, cmd, ...) \
+ (__builtin_constant_p (cmd) \
+ ? __builtin_choose_expression \
+ (__fcntl_is_void (cmd), __fcntl_void (fd, cmd, __VA_ARGS__), \
+ __builtin_choose_expression \
+ (__fcntl_is_int (cmd), __fcntl_int (fd, cmd, __VA_ARGS__), \
+ __builtin_choose_expression \
+ (__fcntl_is_flock_const (cmd), __fcntl_flock_const (fd, cmd, __VA_ARGS__), \
+ __builtin_choose_expression \
+ (__fcntl_is_flock (cmd), __fcntl_flock (fd, cmd, __VA_ARGS__), \
+ __builtin_chose_expression \
+ (__fcntl_is_flock64_const (cmd), __fcntl_flock64_const (fd, cmd, __VA_ARGS__), \
+ __builtin_choose_expression \
+ (__fcntl_is_flock64 (cmd), __fcntl_flock64 (fd, cmd, __VA_ARGS__), \
+ __builtin_choose_expression \
+ (__fcntl_is_flock32_const (cmd), __fcntl_flock32_const (fd, cmd, __VA_ARGS__), \
+ __builtin_choose_expression \
+ (__fcntl_is_flock32 (cmd), __fcntl_flock32 (fd, cmd, __VA_ARGS__), \
+ __fcntl_unchecked (fd, cmd, __VA_ARGS__))))))))) \
+ : __fcntl_unchecked (fd, cmd, __VA_ARGS__))
diff --git a/io/fcntl.h b/io/fcntl.h
index cb706b4f0f..79887db6f8 100644
--- a/io/fcntl.h
+++ b/io/fcntl.h
@@ -314,6 +314,11 @@ extern int posix_fallocate64 (int __fd, off64_t __offset, off64_t __len);
# include <bits/fcntl2.h>
#endif
+# if !defined (__cplusplus) && __USE_FORTIFY_LEVEL > 0 \
+ && defined __fortify_function && defined __va_arg_pack_len
+# include <bits/fcntl3.h>
+#endif
+
__END_DECLS
#endif /* fcntl.h */
next prev parent reply other threads:[~2023-05-24 8:29 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-19 21:30 [RFC PATCH 0/1] Attempt to detect missing fcntl argument at compile time Sergey Bugaev
2023-05-19 21:30 ` [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments Sergey Bugaev
2023-05-19 21:55 ` Joseph Myers
2023-05-20 11:46 ` Sergey Bugaev
2023-05-20 18:21 ` [RFC PATCH] debug: Add tests for fortified fcntl () Sergey Bugaev
2023-05-23 18:40 ` Adhemerval Zanella Netto
2023-05-23 19:19 ` Sergey Bugaev
2023-05-23 19:48 ` Adhemerval Zanella Netto
2023-05-24 7:15 ` Sergey Bugaev
2023-05-24 12:15 ` Adhemerval Zanella Netto
2023-05-23 19:09 ` [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments Adhemerval Zanella Netto
2023-05-23 19:43 ` Sergey Bugaev
2023-05-23 19:56 ` Adhemerval Zanella Netto
2023-05-23 20:24 ` Sergey Bugaev
2023-05-23 20:44 ` Sergey Bugaev
2023-05-24 12:04 ` Adhemerval Zanella Netto
2023-05-23 19:15 ` Siddhesh Poyarekar
2023-05-23 20:01 ` Sergey Bugaev
2023-05-23 20:06 ` Sergey Bugaev
2023-05-23 21:46 ` Florian Weimer
2023-05-24 7:31 ` Sergey Bugaev
2023-05-24 8:29 ` Florian Weimer [this message]
2023-05-24 10:51 ` Sergey Bugaev
2023-05-24 11:18 ` Florian Weimer
2023-05-24 11:46 ` Siddhesh Poyarekar
2023-05-24 12:12 ` Andreas Schwab
2023-05-24 12:18 ` Florian Weimer
2023-05-24 12:37 ` Sergey Bugaev
2023-05-24 12:45 ` Florian Weimer
2023-05-24 13:02 ` Sergey Bugaev
2023-05-24 13:18 ` Florian Weimer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87fs7myvxm.fsf@oldenburg.str.redhat.com \
--to=fweimer@redhat.com \
--cc=bugaevc@gmail.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).