From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 2A4933858C54 for ; Wed, 24 May 2023 08:29:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2A4933858C54 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1684916955; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=IGqFg38XeqEIOGUpDcd9UULnXbHNgwkRWJ1toqoKU0Y=; b=OraRitSj/X3o6ldXwzpjLU9KGGyViHGcbTmC+FmdhoGycFeogmS2bibwZo0d83+RZiSPRM bw9VP+ZjRrtOv054AHarm3MCA6K5RL/GdjsPrxAhRrZIfldYw/qh1gX1MwesNEqHU9CWLS GQ0jSiVJ809KTVGiJdJwVi9berg130A= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-645-0U7UFHeaPsmnztatleBrww-1; Wed, 24 May 2023 04:29:12 -0400 X-MC-Unique: 0U7UFHeaPsmnztatleBrww-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 119A6811E78; Wed, 24 May 2023 08:29:12 +0000 (UTC) Received: from oldenburg.str.redhat.com (unknown [10.2.16.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 20BCC1121314; Wed, 24 May 2023 08:29:11 +0000 (UTC) From: Florian Weimer To: Sergey Bugaev via Libc-alpha Cc: Sergey Bugaev Subject: Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments References: <20230519213059.3812385-1-bugaevc@gmail.com> <20230519213059.3812385-2-bugaevc@gmail.com> <871qj6202r.fsf@mid.deneb.enyo.de> Date: Wed, 24 May 2023 10:29:09 +0200 In-Reply-To: (Sergey Bugaev via Libc-alpha's message of "Wed, 24 May 2023 10:31:26 +0300") Message-ID: <87fs7myvxm.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="=-=-=" X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable * Sergey Bugaev via Libc-alpha: > Hello, > > On Wed, May 24, 2023 at 12:46=E2=80=AFAM Florian Weimer 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 () =3D=3D 1, you cannot do: > > int arg =3D __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_ARG= S__), \ + __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) =3D=3D F_SETLK64 || (cmd) =3D=3D F_SETLKW64) +# define __fcntl_is_flock64(cmd) ((cmd) =3D=3D 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 --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=fcntl.patch commit badf4c4724dcd838d27b40f92eb7d0039a968793 Author: Florian Weimer 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 + . */ + +#ifndef _FCNTL_H +# error "Never include directly; use 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 #endif +# if !defined (__cplusplus) && __USE_FORTIFY_LEVEL > 0 \ + && defined __fortify_function && defined __va_arg_pack_len +# include +#endif + __END_DECLS #endif /* fcntl.h */ --=-=-=--