* Re: [PATCH v6 00/15] integrity: Introduce the Integrity Digest Cache
From: Roberto Sassu @ 2024-11-26 10:41 UTC (permalink / raw)
To: Eric Snowberg
Cc: Mimi Zohar, Dmitry Kasatkin, corbet@lwn.net, mcgrof@kernel.org,
petr.pavlu@suse.com, samitolvanen@google.com,
da.gomez@samsung.com, Andrew Morton, paul@paul-moore.com,
jmorris@namei.org, serge@hallyn.com, shuah@kernel.org,
mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com,
linux-integrity@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
linux-modules@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kselftest@vger.kernel.org, wufan@linux.microsoft.com,
pbrobinson@gmail.com, zbyszek@in.waw.pl, hch@lst.de,
mjg59@srcf.ucam.org, pmatilai@redhat.com, jannh@google.com,
dhowells@redhat.com, jikos@kernel.org, mkoutny@suse.com,
ppavlu@suse.com, petr.vorel@gmail.com, mzerqung@0pointer.de,
kgold@linux.ibm.com, Roberto Sassu
In-Reply-To: <C4BE31F8-1FA3-4AD1-A712-ED2AA7E61E96@oracle.com>
On Tue, 2024-11-26 at 00:13 +0000, Eric Snowberg wrote:
>
> > On Nov 19, 2024, at 3:49 AM, Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
> >
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> >
> > The Integrity Digest Cache can also help IMA for appraisal. IMA can simply
> > lookup the calculated digest of an accessed file in the list of digests
> > extracted from package headers, after verifying the header signature. It is
> > sufficient to verify only one signature for all files in the package, as
> > opposed to verifying a signature for each file.
>
> Is there a way to maintain integrity over time? Today if a CVE is discovered
> in a signed program, the program hash can be added to the blacklist keyring.
> Later if IMA appraisal is used, the signature validation will fail just for that
> program. With the Integrity Digest Cache, is there a way to do this?
As far as I can see, the ima_check_blacklist() call is before
ima_appraise_measurement(). If it fails, appraisal with the Integrity
Digest Cache will not be done.
In the future, we might use the Integrity Digest Cache for blacklists
too. Since a digest cache is reset on a file/directory change, IMA
would have to revalidate the program digest against a new digest cache.
Thanks
Roberto
^ permalink raw reply
* Re: [PATCH v6 07/15] digest_cache: Allow registration of digest list parsers
From: Roberto Sassu @ 2024-11-26 10:25 UTC (permalink / raw)
To: Luis Chamberlain
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, petr.pavlu,
samitolvanen, da.gomez, akpm, paul, jmorris, serge, shuah,
mcoquelin.stm32, alexandre.torgue, linux-integrity, linux-doc,
linux-kernel, linux-api, linux-modules, linux-security-module,
linux-kselftest, wufan, pbrobinson, zbyszek, hch, mjg59, pmatilai,
jannh, dhowells, jikos, mkoutny, ppavlu, petr.vorel, mzerqung,
kgold, Roberto Sassu
In-Reply-To: <Z0UN9ub0iztWvgLi@bombadil.infradead.org>
On Mon, 2024-11-25 at 15:53 -0800, Luis Chamberlain wrote:
> On Tue, Nov 19, 2024 at 11:49:14AM +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > Introduce load_parser() to load a kernel module containing a
> > parser for the requested digest list format (compressed kernel modules are
> > supported). Kernel modules are searched in the
> > /lib/modules/<kernel ver>/security/integrity/digest_cache directory.
> >
> > load_parser() calls ksys_finit_module() to load a kernel module directly
> > from the kernel. request_module() cannot be used at this point, since the
> > reference digests of modprobe and the linked libraries (required for IMA
> > appraisal) might not be yet available, resulting in modprobe execution
> > being denied.
>
> You are doing a full solution implementation of loading modules in-kernel.
> Appraisals of modules is just part of the boot process, some module
> loading may need firmware to loading to get some functinality to work
> for example some firmware to get a network device up or a GPU driver.
> So module loading alone is not the only thing which may require
> IMA appraisal, and this solution only addresses modules. There are other
> things which may be needed other than firmware, eBPF programs are
> another example.
Firmware, eBPF programs and so on are supposed to be verified with
digest lists (or alternative methods, such as file signatures), once
the required digest list parsers are loaded.
The parser is an exceptional case, because user space cannot be
executed at this point. Once the parsers are loaded, verification of
everything else proceeds as normal. Fortunately, in most cases kernel
modules are signed, so digest lists are not required to verify them.
> It sounds more like you want to provide or extend LSM hooks fit your
> architecture and make kernel_read_file() LSM hooks optionally use it to
> fit this model.
As far as the LSM infrastructure is concerned, I'm not adding new LSM
hooks, nor extending/modifying the existing ones. The operations the
Integrity Digest Cache is doing match the usage expectation by LSM (net
denying access, as discussed with Paul Moore).
The Integrity Digest Cache is supposed to be used as a supporting tool
for other LSMs to do regular access control based on file data and
metadata integrity. In doing that, it still needs the LSM
infrastructure to notify about filesystem changes, and to store
additional information in the inode and file descriptor security blobs.
The kernel_post_read_file LSM hook should be implemented by another LSM
to verify the integrity of a digest list, when the Integrity Digest
Cache calls kernel_read_file() to read that digest list. That LSM is
also responsible to provide the result of the integrity verification to
the Integrity Digest Cache, so that the latter can give this
information back to whoever wants to do a digest lookup from that
digest list and also wants to know whether or not the digest list was
authentic.
> Because this is just for a *phase* in boot, which you've caught because
> a catch-22 situaton, where you didn't have your parsers loaded. Which is
> just a reflection that you hit that snag. It doesn't prove all snags
> will be caught yet.
Yes, that didn't happen earlier, since all the parsers were compiled
built-in in the kernel. The Integrity Digest Cache already has a
deadlock avoidance mechanism for digest lists.
Supporting kernel modules opened the road for new deadlocks, since one
can ask a digest list to verify a kernel module, but that digest list
requires the same kernel module. That is why the in-kernel mechanism is
100% reliable, because the Integrity Digest Cache marks the file
descriptors it opens, and can recognize them, when those file
descriptors are passed back to it by other LSMs (e.g. through the
kernel_post_read_file LSM hook).
> And you only want to rely on this .. in-kernel loading solution only
> early on boot, is there a way to change this over to enable regular
> operation later?
User space can voluntarily load new digest list parsers, but the
Integrity Digest Cache cannot rely on it to be done. Also, using
request_module() does not seem a good idea, since it wouldn't allow the
Integrity Digest Cache to mark the file descriptor of kernel modules,
and thus the Integrity Digest Cache could not determine whether or not
a deadlock is happening.
Thanks
Roberto
^ permalink raw reply
* Re: [PATCH v6 02/15] module: Introduce ksys_finit_module()
From: Roberto Sassu @ 2024-11-26 7:56 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Christoph Hellwig, zohar, dmitry.kasatkin, eric.snowberg, corbet,
petr.pavlu, samitolvanen, da.gomez, akpm, paul, jmorris, serge,
shuah, mcoquelin.stm32, alexandre.torgue, linux-integrity,
linux-doc, linux-kernel, linux-api, linux-modules,
linux-security-module, linux-kselftest, wufan, pbrobinson,
zbyszek, mjg59, pmatilai, jannh, dhowells, jikos, mkoutny, ppavlu,
petr.vorel, mzerqung, kgold, Roberto Sassu
In-Reply-To: <Z0UK4Zm85GzeLt20@bombadil.infradead.org>
On Mon, 2024-11-25 at 15:40 -0800, Luis Chamberlain wrote:
> On Wed, Nov 20, 2024 at 10:16:23AM +0100, Roberto Sassu wrote:
> > Again, maybe I misunderstood, but I'm not introducing any functional
> > change to the current behavior, the kernel side also provides a file
> > descriptor and module arguments as user space would do (e.g. by
> > executing insmod).
>
> The commit log does an extremely poor job to make this clear, all you
> are doing here is adding a helper. The commit log should be super clear
> on that and it is not.
Correct, yes. Will do a much better and clear commit message for next
version.
Thanks
Roberto
^ permalink raw reply
* Re: [PATCH net-next v2 11/12] net: homa: create homa_plumbing.c homa_utils.c
From: D. Wythe @ 2024-11-26 5:32 UTC (permalink / raw)
To: John Ousterhout, netdev, linux-api
In-Reply-To: <20241111234006.5942-12-ouster@cs.stanford.edu>
On 11/12/24 7:40 AM, John Ousterhout wrote:
> homa_plumbing.c contains functions that connect Homa to the rest of
> the Linux kernel, such as dispatch tables used by Linux and the
> top-level functions that Linux invokes from those dispatch tables.
>
> homa_utils.c contains a few odds and ends, such as code to initialize
> and destroy struct homa's.
>
> Signed-off-by: John Ousterhout <ouster@cs.stanford.edu>
> ---
> net/homa/homa_plumbing.c | 965 +++++++++++++++++++++++++++++++++++++++
> net/homa/homa_utils.c | 150 ++++++
> 2 files changed, 1115 insertions(+)
> create mode 100644 net/homa/homa_plumbing.c
> create mode 100644 net/homa/homa_utils.c
>
> diff --git a/net/homa/homa_plumbing.c b/net/homa/homa_plumbing.c
> new file mode 100644
> index 000000000000..afd3a9cc97ba
> --- /dev/null
> +++ b/net/homa/homa_plumbing.c
> @@ -0,0 +1,965 @@
> +// SPDX-License-Identifier: BSD-2-Clause
> +
> +/* This file consists mostly of "glue" that hooks Homa into the rest of
> + * the Linux kernel. The guts of the protocol are in other files.
> + */
> +
> +#include "homa_impl.h"
> +#include "homa_peer.h"
> +#include "homa_pool.h"
> +
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_AUTHOR("John Ousterhout");
> +MODULE_DESCRIPTION("Homa transport protocol");
> +MODULE_VERSION("0.01");
> +
> +/* Not yet sure what these variables are for */
> +static long sysctl_homa_mem[3] __read_mostly;
> +static int sysctl_homa_rmem_min __read_mostly;
> +static int sysctl_homa_wmem_min __read_mostly;
> +
> +/* Global data for Homa. Never reference homa_data directory. Always use
> + * the homa variable instead; this allows overriding during unit tests.
> + */
> +static struct homa homa_data;
> +struct homa *homa = &homa_data;
> +
> +/* True means that the Homa module is in the process of unloading itself,
> + * so everyone should clean up.
> + */
> +static bool exiting;
> +
> +/* Thread that runs timer code to detect lost packets and crashed peers. */
> +static struct task_struct *timer_kthread;
> +
> +/* This structure defines functions that handle various operations on
> + * Homa sockets. These functions are relatively generic: they are called
> + * to implement top-level system calls. Many of these operations can
> + * be implemented by PF_INET6 functions that are independent of the
> + * Homa protocol.
> + */
> +static const struct proto_ops homa_proto_ops = {
> + .family = PF_INET,
> + .owner = THIS_MODULE,
> + .release = inet_release,
> + .bind = homa_bind,
> + .connect = inet_dgram_connect,
> + .socketpair = sock_no_socketpair,
> + .accept = sock_no_accept,
> + .getname = inet_getname,
> + .poll = homa_poll,
> + .ioctl = inet_ioctl,
> + .listen = sock_no_listen,
> + .shutdown = homa_shutdown,
> + .setsockopt = sock_common_setsockopt,
> + .getsockopt = sock_common_getsockopt,
> + .sendmsg = inet_sendmsg,
> + .recvmsg = inet_recvmsg,
> + .mmap = sock_no_mmap,
> + .set_peek_off = sk_set_peek_off,
> +};
> +
> +static const struct proto_ops homav6_proto_ops = {
> + .family = PF_INET6,
> + .owner = THIS_MODULE,
> + .release = inet6_release,
> + .bind = homa_bind,
> + .connect = inet_dgram_connect,
> + .socketpair = sock_no_socketpair,
> + .accept = sock_no_accept,
> + .getname = inet6_getname,
> + .poll = homa_poll,
> + .ioctl = inet6_ioctl,
> + .listen = sock_no_listen,
> + .shutdown = homa_shutdown,
> + .setsockopt = sock_common_setsockopt,
> + .getsockopt = sock_common_getsockopt,
> + .sendmsg = inet_sendmsg,
> + .recvmsg = inet_recvmsg,
> + .mmap = sock_no_mmap,
> + .set_peek_off = sk_set_peek_off,
> +};
> +
> +/* This structure also defines functions that handle various operations
> + * on Homa sockets. However, these functions are lower-level than those
> + * in homa_proto_ops: they are specific to the PF_INET or PF_INET6
> + * protocol family, and in many cases they are invoked by functions in
> + * homa_proto_ops. Most of these functions have Homa-specific implementations.
> + */
> +static struct proto homa_prot = {
> + .name = "HOMA",
> + .owner = THIS_MODULE,
> + .close = homa_close,
> + .connect = ip4_datagram_connect,
> + .disconnect = homa_disconnect,
> + .ioctl = homa_ioctl,
> + .init = homa_socket,
> + .destroy = NULL,
> + .setsockopt = homa_setsockopt,
> + .getsockopt = homa_getsockopt,
> + .sendmsg = homa_sendmsg,
> + .recvmsg = homa_recvmsg,
> + .backlog_rcv = homa_backlog_rcv,
> + .hash = homa_hash,
> + .unhash = homa_unhash,
> + .get_port = homa_get_port,
> + .sysctl_mem = sysctl_homa_mem,
> + .sysctl_wmem = &sysctl_homa_wmem_min,
> + .sysctl_rmem = &sysctl_homa_rmem_min,
> + .obj_size = sizeof(struct homa_sock),
> + .no_autobind = 1,
> +};
> +
> +static struct proto homav6_prot = {
> + .name = "HOMAv6",
> + .owner = THIS_MODULE,
> + .close = homa_close,
> + .connect = ip6_datagram_connect,
> + .disconnect = homa_disconnect,
> + .ioctl = homa_ioctl,
> + .init = homa_socket,
> + .destroy = NULL,
> + .setsockopt = homa_setsockopt,
> + .getsockopt = homa_getsockopt,
> + .sendmsg = homa_sendmsg,
> + .recvmsg = homa_recvmsg,
> + .backlog_rcv = homa_backlog_rcv,
> + .hash = homa_hash,
> + .unhash = homa_unhash,
> + .get_port = homa_get_port,
> + .sysctl_mem = sysctl_homa_mem,
> + .sysctl_wmem = &sysctl_homa_wmem_min,
> + .sysctl_rmem = &sysctl_homa_rmem_min,
> +
> + /* IPv6 data comes *after* Homa's data, and isn't included in
> + * struct homa_sock.
> + */
> + .obj_size = sizeof(struct homa_sock) + sizeof(struct ipv6_pinfo),
The implementation of inet6_sk_generic() has already changed, you should set
.ipv6_pinfo_offset.
> + .no_autobind = 1,
> +};
> +
> +/* Top-level structure describing the Homa protocol. */
> +static struct inet_protosw homa_protosw = {
> + .type = SOCK_DGRAM,
> + .protocol = IPPROTO_HOMA,
> + .prot = &homa_prot,
> + .ops = &homa_proto_ops,
> + .flags = INET_PROTOSW_REUSE,
> +};
> +
> +static struct inet_protosw homav6_protosw = {
> + .type = SOCK_DGRAM,
> + .protocol = IPPROTO_HOMA,
> + .prot = &homav6_prot,
> + .ops = &homav6_proto_ops,
> + .flags = INET_PROTOSW_REUSE,
> +};
> +
> +/* This structure is used by IP to deliver incoming Homa packets to us. */
> +static struct net_protocol homa_protocol = {
> + .handler = homa_softirq,
> + .err_handler = homa_err_handler_v4,
> + .no_policy = 1,
> +};
> +
> +static struct inet6_protocol homav6_protocol = {
> + .handler = homa_softirq,
> + .err_handler = homa_err_handler_v6,
> + .flags = INET6_PROTO_NOPOLICY | INET6_PROTO_FINAL,
> +};
> +
> +/* Sizes of the headers for each Homa packet type, in bytes. */
> +static __u16 header_lengths[] = {
> + sizeof32(struct data_header),
> + 0,
> + sizeof32(struct resend_header),
> + sizeof32(struct unknown_header),
> + sizeof32(struct busy_header),
> + 0,
> + sizeof32(struct common_header),
> + sizeof32(struct need_ack_header),
> + sizeof32(struct ack_header)
> +};
> +
> +static DECLARE_COMPLETION(timer_thread_done);
> +
> +/**
> + * homa_load() - invoked when this module is loaded into the Linux kernel
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +static int __init homa_load(void)
> +{
> + int status;
> +
> + pr_notice("Homa module loading\n");
> + pr_notice("Homa structure sizes: data_header %u, seg_header %u, ack %u, peer %u, ip_hdr %u flowi %u ipv6_hdr %u, flowi6 %u tcp_sock %u homa_rpc %u sk_buff %u rcvmsg_control %u union sockaddr_in_union %u HOMA_MAX_BPAGES %u NR_CPUS %u nr_cpu_ids %u, MAX_NUMNODES %d\n",
> + sizeof32(struct data_header),
> + sizeof32(struct seg_header),
> + sizeof32(struct homa_ack),
> + sizeof32(struct homa_peer),
> + sizeof32(struct iphdr),
> + sizeof32(struct flowi),
> + sizeof32(struct ipv6hdr),
> + sizeof32(struct flowi6),
> + sizeof32(struct tcp_sock),
> + sizeof32(struct homa_rpc),
> + sizeof32(struct sk_buff),
> + sizeof32(struct homa_recvmsg_args),
> + sizeof32(union sockaddr_in_union),
> + HOMA_MAX_BPAGES,
> + NR_CPUS,
> + nr_cpu_ids,
> + MAX_NUMNODES);
> + status = proto_register(&homa_prot, 1);
> + if (status != 0) {
> + pr_err("proto_register failed for homa_prot: %d\n", status);
> + goto out;
> + }
> + status = proto_register(&homav6_prot, 1);
> + if (status != 0) {
> + pr_err("proto_register failed for homav6_prot: %d\n", status);
> + goto out;
> + }
> + inet_register_protosw(&homa_protosw);
> + inet6_register_protosw(&homav6_protosw);
better to check the retval of inet6_register_protosw().
> + status = inet_add_protocol(&homa_protocol, IPPROTO_HOMA);
> + if (status != 0) {
> + pr_err("inet_add_protocol failed in %s: %d\n", __func__,
> + status);
> + goto out_cleanup;
> + }
> + status = inet6_add_protocol(&homav6_protocol, IPPROTO_HOMA);
> + if (status != 0) {
> + pr_err("inet6_add_protocol failed in %s: %d\n", __func__,
> + status);
> + goto out_cleanup;
> + }
> +
> + status = homa_init(homa);
> + if (status)
> + goto out_cleanup;
> +
> + timer_kthread = kthread_run(homa_timer_main, homa, "homa_timer");
> + if (IS_ERR(timer_kthread)) {
> + status = PTR_ERR(timer_kthread);
> + pr_err("couldn't create homa pacer thread: error %d\n",
> + status);
> + timer_kthread = NULL;
> + goto out_cleanup;
> + }
> +
> + return 0;
> +
> +out_cleanup:
> + homa_destroy(homa);
> + inet_del_protocol(&homa_protocol, IPPROTO_HOMA);
> + inet_unregister_protosw(&homa_protosw);
> + inet6_del_protocol(&homav6_protocol, IPPROTO_HOMA);
> + inet6_unregister_protosw(&homav6_protosw);
> + proto_unregister(&homa_prot);
> + proto_unregister(&homav6_prot);
It's a bit strange for me that this relies on a premise: that every reverse operation can correctly
identify whether the corresponding forward operation has been executed. Currently, perhaps every
function includes this capability. It's up to you, I don't insist.
> +out:
> + return status;
> +}
> +
> +/**
> + * homa_unload() - invoked when this module is unloaded from the Linux kernel.
> + */
> +static void __exit homa_unload(void)
> +{
> + pr_notice("Homa module unloading\n");
> + exiting = true;
> +
> + if (timer_kthread)
> + wake_up_process(timer_kthread);
> + wait_for_completion(&timer_thread_done);
> + homa_destroy(homa);
> + inet_del_protocol(&homa_protocol, IPPROTO_HOMA);
> + inet_unregister_protosw(&homa_protosw);
> + inet6_del_protocol(&homav6_protocol, IPPROTO_HOMA);
> + inet6_unregister_protosw(&homav6_protosw);
> + proto_unregister(&homa_prot);
> + proto_unregister(&homav6_prot);
> +}
> +
> +module_init(homa_load);
> +module_exit(homa_unload);
Perhaps you can try adding MODULE_ALIAS_NET_PF_PROTO_TYPE so that the kernel will automatically load
the module when creating IPPROTO_HOMA socket. A functional suggestion, It's up to you.
> +
> +/**
> + * homa_bind() - Implements the bind system call for Homa sockets: associates
> + * a well-known service port with a socket. Unlike other AF_INET6 protocols,
> + * there is no need to invoke this system call for sockets that are only
> + * used as clients.
> + * @sock: Socket on which the system call was invoked.
> + * @addr: Contains the desired port number.
> + * @addr_len: Number of bytes in uaddr.
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
> +{
> + struct homa_sock *hsk = homa_sk(sock->sk);
> + union sockaddr_in_union *addr_in = (union sockaddr_in_union *)addr;
> + int port = 0;
> +
> + if (unlikely(addr->sa_family != sock->sk->sk_family))
> + return -EAFNOSUPPORT;
> + if (addr_in->in6.sin6_family == AF_INET6) {
> + if (addr_len < sizeof(struct sockaddr_in6))
> + return -EINVAL;
> + port = ntohs(addr_in->in4.sin_port);
> + } else if (addr_in->in4.sin_family == AF_INET) {
> + if (addr_len < sizeof(struct sockaddr_in))
> + return -EINVAL;
> + port = ntohs(addr_in->in6.sin6_port);
> + }
> + return homa_sock_bind(homa->port_map, hsk, port);
> +}
Is binding multiple times legal? For example, bind 80 first and then bind 8080. If not, I think
you might need to check the inet_num.
> +
> +/**
> + * homa_close() - Invoked when close system call is invoked on a Homa socket.
> + * @sk: Socket being closed
> + * @timeout: ??
> + */
> +void homa_close(struct sock *sk, long timeout)
> +{
> + struct homa_sock *hsk = homa_sk(sk);
> +
> + homa_sock_destroy(hsk);
> + sk_common_release(sk);
> +}
> +
> +/**
> + * homa_shutdown() - Implements the shutdown system call for Homa sockets.
> + * @sock: Socket to shut down.
> + * @how: Ignored: for other sockets, can independently shut down
> + * sending and receiving, but for Homa any shutdown will
> + * shut down everything.
> + *
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_shutdown(struct socket *sock, int how)
> +{
> + homa_sock_shutdown(homa_sk(sock->sk));
> + return 0;
> +}
> +
> +/**
> + * homa_disconnect() - Invoked when disconnect system call is invoked on a
> + * Homa socket.
> + * @sk: Socket to disconnect
> + * @flags: ??
> + *
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_disconnect(struct sock *sk, int flags)
> +{
> + pr_warn("unimplemented disconnect invoked on Homa socket\n");
> + return -EINVAL;
> +}
> +
> +/**
> + * homa_ioctl() - Implements the ioctl system call for Homa sockets.
> + * @sk: Socket on which the system call was invoked.
> + * @cmd: Identifier for a particular ioctl operation.
> + * @karg: Operation-specific argument; typically the address of a block
> + * of data in user address space.
> + *
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_ioctl(struct sock *sk, int cmd, int *karg)
> +{
> + return -EINVAL;
> +}
> +
> +/**
> + * homa_socket() - Implements the socket(2) system call for sockets.
> + * @sk: Socket on which the system call was invoked. The non-Homa
> + * parts have already been initialized.
> + *
> + * Return: always 0 (success).
> + */
> +int homa_socket(struct sock *sk)
> +{
> + struct homa_sock *hsk = homa_sk(sk);
> +
> + homa_sock_init(hsk, homa)
I noticed that homa_sock_init() contains a memory allocation action, perhaps you should add a return
value check.
-> hsk->buffer_pool = kzalloc(sizeof(*hsk->buffer_pool), GFP_KERNEL);
> + return 0;
> +}
> +
> +/**
> + * homa_setsockopt() - Implements the getsockopt system call for Homa sockets.
> + * @sk: Socket on which the system call was invoked.
> + * @level: Level at which the operation should be handled; will always
> + * be IPPROTO_HOMA.
> + * @optname: Identifies a particular setsockopt operation.
> + * @optval: Address in user space of information about the option.
> + * @optlen: Number of bytes of data at @optval.
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_setsockopt(struct sock *sk, int level, int optname, sockptr_t optval,
> + unsigned int optlen)
> +{
> + struct homa_sock *hsk = homa_sk(sk);
> + struct homa_set_buf_args args;
> + int ret;
> +
> + if (level != IPPROTO_HOMA || optname != SO_HOMA_SET_BUF ||
> + optlen != sizeof(struct homa_set_buf_args))
> + return -EINVAL;
> +
> + if (copy_from_sockptr(&args, optval, optlen))
> + return -EFAULT;
> +
> + /* Do a trivial test to make sure we can at least write the first
> + * page of the region.
> + */
> + if (copy_to_user((__force void __user *)args.start, &args, sizeof(args)))
> + return -EFAULT;
> +
> + homa_sock_lock(hsk, "homa_setsockopt SO_HOMA_SET_BUF");
> + ret = homa_pool_init(hsk, (__force void __user *)args.start, args.length);
> + homa_sock_unlock(hsk);
> + return ret;
> +}
> +
> +/**
> + * homa_getsockopt() - Implements the getsockopt system call for Homa sockets.
> + * @sk: Socket on which the system call was invoked.
> + * @level: ??
> + * @optname: Identifies a particular setsockopt operation.
> + * @optval: Address in user space where the option's value should be stored.
> + * @option: ??.
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_getsockopt(struct sock *sk, int level, int optname,
> + char __user *optval, int __user *option)
> +{
> + pr_warn("unimplemented getsockopt invoked on Homa socket: level %d, optname %d\n",
> + level, optname);
> + return -EINVAL;
> +}
> +
> +/**
> + * homa_sendmsg() - Send a request or response message on a Homa socket.
> + * @sk: Socket on which the system call was invoked.
> + * @msg: Structure describing the message to send; the msg_control
> + * field points to additional information.
> + * @length: Number of bytes of the message.
> + * Return: 0 on success, otherwise a negative errno.
> + */
> +int homa_sendmsg(struct sock *sk, struct msghdr *msg, size_t length)
> +{
> + struct homa_sock *hsk = homa_sk(sk);
> + struct homa_sendmsg_args args;
> + int result = 0;
> + struct homa_rpc *rpc = NULL;
> + union sockaddr_in_union *addr = (union sockaddr_in_union *)msg->msg_name;
> +
> + if (unlikely(!msg->msg_control_is_user)) {
> + result = -EINVAL;
> + goto error;
> + }
> + if (unlikely(copy_from_user(&args,
> + (__force void __user *)msg->msg_control,
> + sizeof(args)))) {
> + result = -EFAULT;
> + goto error;
> + }
> + if (addr->in6.sin6_family != sk->sk_family) {
> + result = -EAFNOSUPPORT;
> + goto error;
> + }
> + if (msg->msg_namelen < sizeof(struct sockaddr_in) ||
> + (msg->msg_namelen < sizeof(struct sockaddr_in6) &&
> + addr->in6.sin6_family == AF_INET6)) {
> + result = -EINVAL;
> + goto error;
> + }
> +
> + if (!args.id) {
> + /* This is a request message. */
> + rpc = homa_rpc_new_client(hsk, addr);
> + if (IS_ERR(rpc)) {
> + result = PTR_ERR(rpc);
> + rpc = NULL;
> + goto error;
> + }
> + rpc->completion_cookie = args.completion_cookie;
> + result = homa_message_out_fill(rpc, &msg->msg_iter, 1);
> + if (result)
> + goto error;
> + args.id = rpc->id;
> + homa_rpc_unlock(rpc);
> + rpc = NULL;
> +
> + if (unlikely(copy_to_user((__force void __user *)msg->msg_control,
> + &args, sizeof(args)))) {
> + rpc = homa_find_client_rpc(hsk, args.id);
> + result = -EFAULT;
> + goto error;
> + }
> + } else {
> + /* This is a response message. */
> + struct in6_addr canonical_dest;
> +
> + if (args.completion_cookie != 0) {
> + result = -EINVAL;
> + goto error;
> + }
> + canonical_dest = canonical_ipv6_addr(addr);
> +
> + rpc = homa_find_server_rpc(hsk, &canonical_dest,
> + ntohs(addr->in6.sin6_port), args.id);
> + if (!rpc)
> + /* Return without an error if the RPC doesn't exist;
> + * this could be totally valid (e.g. client is
> + * no longer interested in it).
> + */
> + return 0;
> + if (rpc->error) {
> + result = rpc->error;
> + goto error;
> + }
> + if (rpc->state != RPC_IN_SERVICE) {
> + homa_rpc_unlock(rpc);
> + rpc = NULL;
> + result = -EINVAL;
> + goto error;
> + }
> + rpc->state = RPC_OUTGOING;
> +
> + result = homa_message_out_fill(rpc, &msg->msg_iter, 1);
> + if (result && rpc->state != RPC_DEAD)
> + goto error;
> + homa_rpc_unlock(rpc);
> + }
> + return 0;
> +
> +error:
> + if (rpc) {
> + homa_rpc_free(rpc);
> + homa_rpc_unlock(rpc);
> + }
> + return result;
> +}
> +
> +/**
> + * homa_recvmsg() - Receive a message from a Homa socket.
> + * @sk: Socket on which the system call was invoked.
> + * @msg: Controlling information for the receive.
> + * @len: Total bytes of space available in msg->msg_iov; not used.
> + * @flags: Flags from system call; only MSG_DONTWAIT is used.
> + * @addr_len: Store the length of the sender address here
> + * Return: The length of the message on success, otherwise a negative
> + * errno.
> + */
> +int homa_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int flags,
> + int *addr_len)
> +{
> + struct homa_sock *hsk = homa_sk(sk);
> + struct homa_recvmsg_args control;
> + struct homa_rpc *rpc;
> + int result;
> +
> + if (unlikely(!msg->msg_control)) {
> + /* This test isn't strictly necessary, but it provides a
> + * hook for testing kernel call times.
> + */
> + return -EINVAL;
> + }
> + if (msg->msg_controllen != sizeof(control)) {
> + result = -EINVAL;
> + goto done;
> + }
> + if (unlikely(copy_from_user(&control,
> + (__force void __user *)msg->msg_control,
> + sizeof(control)))) {
> + result = -EFAULT;
> + goto done;
> + }
> + control.completion_cookie = 0;
> +
> + if (control.num_bpages > HOMA_MAX_BPAGES ||
> + (control.flags & ~HOMA_RECVMSG_VALID_FLAGS)) {
> + result = -EINVAL;
> + goto done;
> + }
> + homa_pool_release_buffers(hsk->buffer_pool, control.num_bpages,
> + control.bpage_offsets);
> + control.num_bpages = 0;
> +
> + rpc = homa_wait_for_message(hsk, (flags & MSG_DONTWAIT)
> + ? (control.flags | HOMA_RECVMSG_NONBLOCKING)
> + : control.flags, control.id);
> + if (IS_ERR(rpc)) {
> + /* If we get here, it means there was an error that prevented
> + * us from finding an RPC to return. If there's an error in
> + * the RPC itself we won't get here.
> + */
> + result = PTR_ERR(rpc);
> + goto done;
> + }
> + result = rpc->error ? rpc->error : rpc->msgin.length;
> +
> + /* Collect result information. */
> + control.id = rpc->id;
> + control.completion_cookie = rpc->completion_cookie;
> + if (likely(rpc->msgin.length >= 0)) {
> + control.num_bpages = rpc->msgin.num_bpages;
> + memcpy(control.bpage_offsets, rpc->msgin.bpage_offsets,
> + sizeof(control.bpage_offsets));
> + }
> + if (sk->sk_family == AF_INET6) {
> + struct sockaddr_in6 *in6 = msg->msg_name;
> +
> + in6->sin6_family = AF_INET6;
> + in6->sin6_port = htons(rpc->dport);
> + in6->sin6_addr = rpc->peer->addr;
> + *addr_len = sizeof(*in6);
> + } else {
> + struct sockaddr_in *in4 = msg->msg_name;
> +
> + in4->sin_family = AF_INET;
> + in4->sin_port = htons(rpc->dport);
> + in4->sin_addr.s_addr = ipv6_to_ipv4(rpc->peer->addr);
> + *addr_len = sizeof(*in4);
> + }
> +
> + /* This indicates that the application now owns the buffers, so
> + * we won't free them in homa_rpc_free.
> + */
> + rpc->msgin.num_bpages = 0;
> +
> + /* Must release the RPC lock (and potentially free the RPC) before
> + * copying the results back to user space.
> + */
> + if (homa_is_client(rpc->id)) {
> + homa_peer_add_ack(rpc);
> + homa_rpc_free(rpc);
> + } else {
> + if (result < 0)
> + homa_rpc_free(rpc);
> + else
> + rpc->state = RPC_IN_SERVICE;
> + }
> + homa_rpc_unlock(rpc);
> +
> +done:
> + if (unlikely(copy_to_user((__force void __user *)msg->msg_control,
> + &control, sizeof(control)))) {
> + /* Note: in this case the message's buffers will be leaked. */
> + pr_notice("%s couldn't copy back args\n", __func__);
> + result = -EFAULT;
> + }
> +
> + return result;
> +}
> +
> +/**
> + * homa_hash() - Not needed for Homa.
> + * @sk: Socket for the operation
> + * Return: ??
> + */
> +int homa_hash(struct sock *sk)
> +{
> + return 0;
> +}
> +
> +/**
> + * homa_unhash() - Not needed for Homa.
> + * @sk: Socket for the operation
> + */
> +void homa_unhash(struct sock *sk)
> +{
> +}
> +
> +/**
> + * homa_get_port() - It appears that this function is called to assign a
> + * default port for a socket.
> + * @sk: Socket for the operation
> + * @snum: Unclear what this is.
> + * Return: Zero for success, or a negative errno for an error.
> + */
> +int homa_get_port(struct sock *sk, unsigned short snum)
> +{
> + /* Homa always assigns ports immediately when a socket is created,
> + * so there is nothing to do here.
> + */
> + return 0;
> +}
> +
> +/**
> + * homa_softirq() - This function is invoked at SoftIRQ level to handle
> + * incoming packets.
> + * @skb: The incoming packet.
> + * Return: Always 0
> + */
> +int homa_softirq(struct sk_buff *skb)
> +{
> + struct sk_buff *packets, *other_pkts, *next;
> + struct sk_buff **prev_link, **other_link;
> + struct common_header *h;
> + int first_packet = 1;
> + int header_offset;
> + int pull_length;
> +
> + /* skb may actually contain many distinct packets, linked through
> + * skb_shinfo(skb)->frag_list by the Homa GRO mechanism. Make a
> + * pass through the list to process all of the short packets,
> + * leaving the longer packets in the list. Also, perform various
> + * prep/cleanup/error checking functions.
> + */
> + skb->next = skb_shinfo(skb)->frag_list;
> + skb_shinfo(skb)->frag_list = NULL;
> + packets = skb;
> + prev_link = &packets;
> + for (skb = packets; skb; skb = next) {
> + next = skb->next;
> +
> + /* Make the header available at skb->data, even if the packet
> + * is fragmented. One complication: it's possible that the IP
> + * header hasn't yet been removed (this happens for GRO packets
> + * on the frag_list, since they aren't handled explicitly by IP.
> + */
> + header_offset = skb_transport_header(skb) - skb->data;
> + pull_length = HOMA_MAX_HEADER + header_offset;
> + if (pull_length > skb->len)
> + pull_length = skb->len;
> + if (!pskb_may_pull(skb, pull_length))
> + goto discard;
> + if (header_offset)
> + __skb_pull(skb, header_offset);
> +
> + /* Reject packets that are too short or have bogus types. */
> + h = (struct common_header *)skb->data;
> + if (unlikely(skb->len < sizeof(struct common_header) ||
> + h->type < DATA || h->type >= BOGUS ||
> + skb->len < header_lengths[h->type - DATA]))
> + goto discard;
> +
> + if (first_packet)
> + first_packet = 0;
> +
> + /* Process the packet now if it is a control packet or
> + * if it contains an entire short message.
> + */
> + if (h->type != DATA || ntohl(((struct data_header *)h)
> + ->message_length) < 1400) {
> + *prev_link = skb->next;
> + skb->next = NULL;
> + homa_dispatch_pkts(skb, homa);
> + } else {
> + prev_link = &skb->next;
> + }
> + continue;
> +
> +discard:
> + *prev_link = skb->next;
> + kfree_skb(skb);
> + }
> +
> + /* Now process the longer packets. Each iteration of this loop
> + * collects all of the packets for a particular RPC and dispatches
> + * them.
> + */
> + while (packets) {
> + struct in6_addr saddr, saddr2;
> + struct common_header *h2;
> + struct sk_buff *skb2;
> +
> + skb = packets;
> + prev_link = &skb->next;
> + saddr = skb_canonical_ipv6_saddr(skb);
> + other_pkts = NULL;
> + other_link = &other_pkts;
> + h = (struct common_header *)skb->data;
> + for (skb2 = skb->next; skb2; skb2 = next) {
> + next = skb2->next;
> + h2 = (struct common_header *)skb2->data;
> + if (h2->sender_id == h->sender_id) {
> + saddr2 = skb_canonical_ipv6_saddr(skb2);
> + if (ipv6_addr_equal(&saddr, &saddr2)) {
> + *prev_link = skb2;
> + prev_link = &skb2->next;
> + continue;
> + }
> + }
> + *other_link = skb2;
> + other_link = &skb2->next;
> + }
> + *prev_link = NULL;
> + *other_link = NULL;
> + homa_dispatch_pkts(packets, homa);
> + packets = other_pkts;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * homa_backlog_rcv() - Invoked to handle packets saved on a socket's
> + * backlog because it was locked when the packets first arrived.
> + * @sk: Homa socket that owns the packet's destination port.
> + * @skb: The incoming packet. This function takes ownership of the packet
> + * (we'll delete it).
> + *
> + * Return: Always returns 0.
> + */
> +int homa_backlog_rcv(struct sock *sk, struct sk_buff *skb)
> +{
> + pr_warn("unimplemented backlog_rcv invoked on Homa socket\n");
> + kfree_skb(skb);
> + return 0;
> +}
> +
> +/**
> + * homa_err_handler_v4() - Invoked by IP to handle an incoming error
> + * packet, such as ICMP UNREACHABLE.
> + * @skb: The incoming packet.
> + * @info: Information about the error that occurred?
> + *
> + * Return: zero, or a negative errno if the error couldn't be handled here.
> + */
> +int homa_err_handler_v4(struct sk_buff *skb, u32 info)
> +{
> + const struct in6_addr saddr = skb_canonical_ipv6_saddr(skb);
> + const struct iphdr *iph = ip_hdr(skb);
> + int type = icmp_hdr(skb)->type;
> + int code = icmp_hdr(skb)->code;
> +
> + if (type == ICMP_DEST_UNREACH && code == ICMP_PORT_UNREACH) {
> + char *icmp = (char *)icmp_hdr(skb);
> + struct common_header *h;
> +
> + iph = (struct iphdr *)(icmp + sizeof(struct icmphdr));
> + h = (struct common_header *)(icmp + sizeof(struct icmphdr)
> + + iph->ihl * 4);
> + homa_abort_rpcs(homa, &saddr, ntohs(h->dport), -ENOTCONN);
> + } else if (type == ICMP_DEST_UNREACH) {
> + int error;
> +
> + if (code == ICMP_PROT_UNREACH)
> + error = -EPROTONOSUPPORT;
> + else
> + error = -EHOSTUNREACH;
> + homa_abort_rpcs(homa, &saddr, 0, error);
> + } else {
> + pr_notice("%s invoked with info %x, ICMP type %d, ICMP code %d\n",
> + __func__, info, type, code);
> + }
> + return 0;
> +}
> +
> +/**
> + * homa_err_handler_v6() - Invoked by IP to handle an incoming error
> + * packet, such as ICMP UNREACHABLE.
> + * @skb: The incoming packet.
> + * @opt: Not used.
> + * @type: Type of ICMP packet.
> + * @code: Additional information about the error.
> + * @offset: Not used.
> + * @info: Information about the error that occurred?
> + *
> + * Return: zero, or a negative errno if the error couldn't be handled here.
> + */
> +int homa_err_handler_v6(struct sk_buff *skb, struct inet6_skb_parm *opt,
> + u8 type, u8 code, int offset, __be32 info)
> +{
> + const struct ipv6hdr *iph = (const struct ipv6hdr *)skb->data;
> +
> + if (type == ICMPV6_DEST_UNREACH && code == ICMPV6_PORT_UNREACH) {
> + char *icmp = (char *)icmp_hdr(skb);
> + struct common_header *h;
> +
> + iph = (struct ipv6hdr *)(icmp + sizeof(struct icmphdr));
> + h = (struct common_header *)(icmp + sizeof(struct icmphdr)
> + + HOMA_IPV6_HEADER_LENGTH);
> + homa_abort_rpcs(homa, &iph->daddr, ntohs(h->dport), -ENOTCONN);
> + } else if (type == ICMPV6_DEST_UNREACH) {
> + int error;
> +
> + if (code == ICMP_PROT_UNREACH)
> + error = -EPROTONOSUPPORT;
> + else
> + error = -EHOSTUNREACH;
> + homa_abort_rpcs(homa, &iph->daddr, 0, error);
> + }
> + return 0;
> +}
> +
> +/**
> + * homa_poll() - Invoked by Linux as part of implementing select, poll,
> + * epoll, etc.
> + * @file: Open file that is participating in a poll, select, etc.
> + * @sock: A Homa socket, associated with @file.
> + * @wait: This table will be registered with the socket, so that it
> + * is notified when the socket's ready state changes.
> + *
> + * Return: A mask of bits such as EPOLLIN, which indicate the current
> + * state of the socket.
> + */
> +__poll_t homa_poll(struct file *file, struct socket *sock,
> + struct poll_table_struct *wait)
> +{
> + struct sock *sk = sock->sk;
> + __u32 mask;
> +
> + /* It seems to be standard practice for poll functions *not* to
> + * acquire the socket lock, so we don't do it here; not sure
> + * why...
> + */
> +
> + sock_poll_wait(file, sock, wait);
> + mask = POLLOUT | POLLWRNORM;
> +
> + if (!list_empty(&homa_sk(sk)->ready_requests) ||
> + !list_empty(&homa_sk(sk)->ready_responses))
> + mask |= POLLIN | POLLRDNORM;
> + return (__force __poll_t)mask;
> +}
> +
> +/**
> + * homa_hrtimer() - This function is invoked by the hrtimer mechanism to
> + * wake up the timer thread. Runs at IRQ level.
> + * @timer: The timer that triggered; not used.
> + *
> + * Return: Always HRTIMER_RESTART.
> + */
> +enum hrtimer_restart homa_hrtimer(struct hrtimer *timer)
> +{
> + wake_up_process(timer_kthread);
> + return HRTIMER_NORESTART;
> +}
> +
> +/**
> + * homa_timer_main() - Top-level function for the timer thread.
> + * @transport: Pointer to struct homa.
> + *
> + * Return: Always 0.
> + */
> +int homa_timer_main(void *transport)
> +{
> + struct homa *homa = (struct homa *)transport;
> + struct hrtimer hrtimer;
> + ktime_t tick_interval;
> + u64 nsec;
> +
> + hrtimer_init(&hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + hrtimer.function = &homa_hrtimer;
> + nsec = 1000000; /* 1 ms */
> + tick_interval = ns_to_ktime(nsec);
> + while (1) {
> + set_current_state(TASK_UNINTERRUPTIBLE);
> + if (!exiting) {
> + hrtimer_start(&hrtimer, tick_interval, HRTIMER_MODE_REL);
> + schedule();
> + }
> + __set_current_state(TASK_RUNNING);
> + if (exiting)
> + break;
> + homa_timer(homa);
> + }
> + hrtimer_cancel(&hrtimer);
> + kthread_complete_and_exit(&timer_thread_done, 0);
> + return 0;
> +}
> diff --git a/net/homa/homa_utils.c b/net/homa/homa_utils.c
> new file mode 100644
> index 000000000000..905d00c836bd
> --- /dev/null
> +++ b/net/homa/homa_utils.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: BSD-2-Clause
> +
> +/* This file contains miscellaneous utility functions for Homa, such
> + * as initializing and destroying homa structs.
> + */
> +
> +#include "homa_impl.h"
> +#include "homa_peer.h"
> +#include "homa_rpc.h"
> +#include "homa_stub.h"
> +
> +struct completion homa_pacer_kthread_done;
> +
> +/**
> + * homa_init() - Constructor for homa objects.
> + * @homa: Object to initialize.
> + *
> + * Return: 0 on success, or a negative errno if there was an error. Even
> + * if an error occurs, it is safe (and necessary) to call
> + * homa_destroy at some point.
> + */
> +int homa_init(struct homa *homa)
> +{
> + int err;
> +
> + _Static_assert(HOMA_MAX_PRIORITIES >= 8,
> + "homa_init assumes at least 8 priority levels");
> +
> + homa->pacer_kthread = NULL;
> + init_completion(&homa_pacer_kthread_done);
> + atomic64_set(&homa->next_outgoing_id, 2);
> + atomic64_set(&homa->link_idle_time, sched_clock());
> + spin_lock_init(&homa->pacer_mutex);
> + homa->pacer_fifo_fraction = 50;
> + homa->pacer_fifo_count = 1;
> + homa->pacer_wake_time = 0;
> + spin_lock_init(&homa->throttle_lock);
> + INIT_LIST_HEAD_RCU(&homa->throttled_rpcs);
> + homa->throttle_add = 0;
> + homa->throttle_min_bytes = 200;
> + homa->next_client_port = HOMA_MIN_DEFAULT_PORT;
> + homa->port_map = kmalloc(sizeof(*homa->port_map), GFP_KERNEL);
> + if (!homa->port_map) {
> + pr_err("%s couldn't create port_map: kmalloc failure", __func__);
> + return -ENOMEM;
> + }
> + homa_socktab_init(homa->port_map);
> + homa->peers = kmalloc(sizeof(*homa->peers), GFP_KERNEL);
> + if (!homa->peers) {
> + pr_err("%s couldn't create peers: kmalloc failure", __func__);
> + return -ENOMEM;
> + }
> + err = homa_peertab_init(homa->peers);
> + if (err) {
> + pr_err("%s couldn't initialize peer table (errno %d)\n",
> + __func__, -err);
> + return err;
> + }
> +
> + /* Wild guesses to initialize configuration values... */
> + homa->unsched_bytes = 40000;
> + homa->window_param = 100000;
> + homa->link_mbps = 25000;
> + homa->fifo_grant_increment = 10000;
> + homa->grant_fifo_fraction = 50;
> + homa->max_overcommit = 8;
> + homa->max_incoming = 400000;
> + homa->max_rpcs_per_peer = 1;
> + homa->resend_ticks = 5;
> + homa->resend_interval = 5;
> + homa->timeout_ticks = 100;
> + homa->timeout_resends = 5;
> + homa->request_ack_ticks = 2;
> + homa->reap_limit = 10;
> + homa->dead_buffs_limit = 5000;
> + homa->max_dead_buffs = 0;
> + homa->pacer_kthread = kthread_run(homa_pacer_main, homa,
> + "homa_pacer");
> + if (IS_ERR(homa->pacer_kthread)) {
> + err = PTR_ERR(homa->pacer_kthread);
> + homa->pacer_kthread = NULL;
> + pr_err("couldn't create homa pacer thread: error %d\n", err);
> + return err;
> + }
> + homa->pacer_exit = false;
> + homa->max_nic_queue_ns = 5000;
> + homa->ns_per_mbyte = 0;
> + homa->max_gso_size = 10000;
> + homa->gso_force_software = 0;
> + homa->max_gro_skbs = 20;
> + homa->gro_policy = HOMA_GRO_NORMAL;
> + homa->timer_ticks = 0;
> + homa->flags = 0;
> + homa->bpage_lease_usecs = 10000;
> + homa->next_id = 0;
> + homa_outgoing_sysctl_changed(homa);
> + homa_incoming_sysctl_changed(homa);
> + return 0;
> +}
> +
> +/**
> + * homa_destroy() - Destructor for homa objects.
> + * @homa: Object to destroy.
> + */
> +void homa_destroy(struct homa *homa)
> +{
> + if (homa->pacer_kthread) {
> + homa_pacer_stop(homa);
> + wait_for_completion(&homa_pacer_kthread_done);
> + }
> +
> + /* The order of the following statements matters! */
> + if (homa->port_map) {
> + homa_socktab_destroy(homa->port_map);
> + kfree(homa->port_map);
> + homa->port_map = NULL;
> + }
> + if (homa->peers) {
> + homa_peertab_destroy(homa->peers);
> + kfree(homa->peers);
> + homa->peers = NULL;
> + }
> +}
> +
> +/**
> + * homa_spin() - Delay (without sleeping) for a given time interval.
> + * @ns: How long to delay (in nanoseconds)
> + */
> +void homa_spin(int ns)
> +{
> + __u64 end;
> +
> + end = sched_clock() + ns;
> + while (sched_clock() < end)
> + /* Empty loop body.*/
> + ;
> +}
> +
> +/**
> + * homa_throttle_lock_slow() - This function implements the slow path for
> + * acquiring the throttle lock. It is invoked when the lock isn't immediately
> + * available. It waits for the lock, but also records statistics about
> + * the waiting time.
> + * @homa: Overall data about the Homa protocol implementation.
> + */
> +void homa_throttle_lock_slow(struct homa *homa)
> + __acquires(&homa->throttle_lock)
> +{
> + spin_lock_bh(&homa->throttle_lock);
> +}
^ permalink raw reply
* Re: [PATCH net-next v2 12/12] net: homa: create Makefile and Kconfig
From: D. Wythe @ 2024-11-26 4:27 UTC (permalink / raw)
To: John Ousterhout, netdev, linux-api
In-Reply-To: <20241111234006.5942-13-ouster@cs.stanford.edu>
On 11/12/24 7:40 AM, John Ousterhout wrote:
> Before this commit the Homa code is "inert": it won't be compiled
> in kernel builds. This commit adds Homa's Makefile and Kconfig, and
> also links Homa into net/Makefile and net/Kconfig, so that Homa
> will be built during kernel builds if enabled (it is disabled by
> default).
>
> Signed-off-by: John Ousterhout <ouster@cs.stanford.edu>
> ---
> MAINTAINERS | 7 +++++++
> net/Kconfig | 1 +
> net/Makefile | 1 +
> net/homa/Kconfig | 19 +++++++++++++++++++
> net/homa/Makefile | 14 ++++++++++++++
> 5 files changed, 42 insertions(+)
> create mode 100644 net/homa/Kconfig
> create mode 100644 net/homa/Makefile
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1389704c7d8d..935d1e995018 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10391,6 +10391,13 @@ F: lib/test_hmm*
> F: mm/hmm*
> F: tools/testing/selftests/mm/*hmm*
>
> +HOMA TRANSPORT PROTOCOL
> +M: John Ousterhout <ouster@cs.stanford.edu>
> +S: Maintained
> +W: https://homa-transport.atlassian.net/wiki/spaces/HOMA/overview
> +F: include/uapi/linux/homa.h
> +F: net/homa/
> +
> HONEYWELL HSC030PA PRESSURE SENSOR SERIES IIO DRIVER
> M: Petre Rodan <petre.rodan@subdimension.ro>
> L: linux-iio@vger.kernel.org
> diff --git a/net/Kconfig b/net/Kconfig
> index a629f92dc86b..ca8551c1a226 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -244,6 +244,7 @@ endif
>
> source "net/dccp/Kconfig"
> source "net/sctp/Kconfig"
> +source "net/homa/Kconfig"
> source "net/rds/Kconfig"
> source "net/tipc/Kconfig"
> source "net/atm/Kconfig"
> diff --git a/net/Makefile b/net/Makefile
> index 65bb8c72a35e..18fa3c323187 100644
> --- a/net/Makefile
> +++ b/net/Makefile
> @@ -44,6 +44,7 @@ obj-y += 8021q/
> endif
> obj-$(CONFIG_IP_DCCP) += dccp/
> obj-$(CONFIG_IP_SCTP) += sctp/
> +obj-$(CONFIG_HOMA) += homa/
A small formatting issue, perhaps you're using spaces?
> obj-$(CONFIG_RDS) += rds/
> obj-$(CONFIG_WIRELESS) += wireless/
> obj-$(CONFIG_MAC80211) += mac80211/
> diff --git a/net/homa/Kconfig b/net/homa/Kconfig
> new file mode 100644
> index 000000000000..8ba81b00d35f
> --- /dev/null
> +++ b/net/homa/Kconfig
> @@ -0,0 +1,19 @@
> +# SPDX-License-Identifier: BSD-2-Clause
> +#
> +# Homa transport protocol
> +#
> +
> +menuconfig HOMA
> + tristate "The Homa transport protocol"
> + depends on INET
> + depends on IPV6
Can HOMA run in an environment without IPv6（IPv4 only)? If so, depends is not suitable here. Perhaps
what you need is to implement different branches in the code using
#if IS_ENABLED(CONFIG_IPV6)
> +
> + help
> + Homa is a network transport protocol for communication within
> + a datacenter. It provides significantly lower latency than TCP,
> + particularly for workloads containing a mixture of large and small
> + messages operating at high network utilization. For more information
> + see the homa(7) man page or checkout the Homa Wiki at
> + https://homa-transport.atlassian.net/wiki/spaces/HOMA/overview.
> +
> + If unsure, say N.
> diff --git a/net/homa/Makefile b/net/homa/Makefile
> new file mode 100644
> index 000000000000..3eb192a6ffa6
> --- /dev/null
> +++ b/net/homa/Makefile
> @@ -0,0 +1,14 @@
> +# SPDX-License-Identifier: BSD-2-Clause
> +#
> +# Makefile for the Linux implementation of the Homa transport protocol.
> +
> +obj-$(CONFIG_HOMA) := homa.o
> +homa-y:= homa_incoming.o \
> + homa_outgoing.o \
> + homa_peer.o \
> + homa_pool.o \
> + homa_plumbing.o \
> + homa_rpc.o \
> + homa_sock.o \
> + homa_timer.o \
> + homa_utils.o
^ permalink raw reply
* Re: [PATCH net-next v2 12/12] net: homa: create Makefile and Kconfig
From: D. Wythe @ 2024-11-26 4:13 UTC (permalink / raw)
To: John Ousterhout, kernel test robot; +Cc: netdev, linux-api, oe-kbuild-all
In-Reply-To: <CAGXJAmyeJ2HWxOZj8uY0_Cu47Yy_hAYfSRGmYpNfLGd2PR7Q-Q@mail.gmail.com>
On 11/19/24 5:23 AM, John Ousterhout wrote:
> I believe I have fixed all of the kernel test robot issues reported
> under this subject line.
>
> -John-
>
Hi John,
I guess you might need pass type u64 rather type int as the first argument into do_div().
In fact, this is not a simple warning, as it may cause anomalies.
// int segs;
u64 segs;
do_div(segs, max_seg_data);
// int segs_per_gso
u64 segs_per_gso;
do_div(segs_per_gso, max_seg_data);
D. Wythe
>
> On Wed, Nov 13, 2024 at 5:53 AM kernel test robot <lkp@intel.com> wrote:
>>
>> Hi John,
>>
>> kernel test robot noticed the following build warnings:
>>
>> [auto build test WARNING on net-next/main]
>>
>> url: https://github.com/intel-lab-lkp/linux/commits/John-Ousterhout/net-homa-define-user-visible-API-for-Homa/20241112-074535
>> base: net-next/main
>> patch link: https://lore.kernel.org/r/20241111234006.5942-13-ouster%40cs.stanford.edu
>> patch subject: [PATCH net-next v2 12/12] net: homa: create Makefile and Kconfig
>> config: riscv-randconfig-r112-20241113 (https://download.01.org/0day-ci/archive/20241113/202411132114.VB5yFmtR-lkp@intel.com/config)
>> compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
>> reproduce: (https://download.01.org/0day-ci/archive/20241113/202411132114.VB5yFmtR-lkp@intel.com/reproduce)
>>
>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <lkp@intel.com>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/202411132114.VB5yFmtR-lkp@intel.com/
>>
>> sparse warnings: (new ones prefixed by >>)
>>>> net/homa/homa_sock.c:201:31: sparse: sparse: cast removes address space '__rcu' of expression
>> net/homa/homa_sock.c:248:17: sparse: sparse: context imbalance in 'homa_sock_shutdown' - different lock contexts for basic block
>> net/homa/homa_sock.c:303:21: sparse: sparse: context imbalance in 'homa_sock_bind' - different lock contexts for basic block
>>
>> vim +/__rcu +201 net/homa/homa_sock.c
>>
>> 8ddf00265eb650 John Ousterhout 2024-11-11 183
>> 8ddf00265eb650 John Ousterhout 2024-11-11 184 /*
>> 8ddf00265eb650 John Ousterhout 2024-11-11 185 * homa_sock_unlink() - Unlinks a socket from its socktab and does
>> 8ddf00265eb650 John Ousterhout 2024-11-11 186 * related cleanups. Once this method returns, the socket will not be
>> 8ddf00265eb650 John Ousterhout 2024-11-11 187 * discoverable through the socktab.
>> 8ddf00265eb650 John Ousterhout 2024-11-11 188 */
>> 8ddf00265eb650 John Ousterhout 2024-11-11 189 void homa_sock_unlink(struct homa_sock *hsk)
>> 8ddf00265eb650 John Ousterhout 2024-11-11 190 {
>> 8ddf00265eb650 John Ousterhout 2024-11-11 191 struct homa_socktab *socktab = hsk->homa->port_map;
>> 8ddf00265eb650 John Ousterhout 2024-11-11 192 struct homa_socktab_scan *scan;
>> 8ddf00265eb650 John Ousterhout 2024-11-11 193
>> 8ddf00265eb650 John Ousterhout 2024-11-11 194 /* If any scans refer to this socket, advance them to refer to
>> 8ddf00265eb650 John Ousterhout 2024-11-11 195 * the next socket instead.
>> 8ddf00265eb650 John Ousterhout 2024-11-11 196 */
>> 8ddf00265eb650 John Ousterhout 2024-11-11 197 spin_lock_bh(&socktab->write_lock);
>> 8ddf00265eb650 John Ousterhout 2024-11-11 198 list_for_each_entry(scan, &socktab->active_scans, scan_links) {
>> 8ddf00265eb650 John Ousterhout 2024-11-11 199 if (!scan->next || scan->next->sock != hsk)
>> 8ddf00265eb650 John Ousterhout 2024-11-11 200 continue;
>> 8ddf00265eb650 John Ousterhout 2024-11-11 @201 scan->next = (struct homa_socktab_links *)hlist_next_rcu(&scan
>> 8ddf00265eb650 John Ousterhout 2024-11-11 202 ->next->hash_links);
>> 8ddf00265eb650 John Ousterhout 2024-11-11 203 }
>> 8ddf00265eb650 John Ousterhout 2024-11-11 204 hlist_del_rcu(&hsk->socktab_links.hash_links);
>> 8ddf00265eb650 John Ousterhout 2024-11-11 205 spin_unlock_bh(&socktab->write_lock);
>> 8ddf00265eb650 John Ousterhout 2024-11-11 206 }
>> 8ddf00265eb650 John Ousterhout 2024-11-11 207
>>
>> --
>> 0-DAY CI Kernel Test Service
>> https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v6 00/15] integrity: Introduce the Integrity Digest Cache
From: Eric Snowberg @ 2024-11-26 0:13 UTC (permalink / raw)
To: Roberto Sassu
Cc: Mimi Zohar, Dmitry Kasatkin, corbet@lwn.net, mcgrof@kernel.org,
petr.pavlu@suse.com, samitolvanen@google.com,
da.gomez@samsung.com, Andrew Morton, paul@paul-moore.com,
jmorris@namei.org, serge@hallyn.com, shuah@kernel.org,
mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com,
linux-integrity@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
linux-modules@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kselftest@vger.kernel.org, wufan@linux.microsoft.com,
pbrobinson@gmail.com, zbyszek@in.waw.pl, hch@lst.de,
mjg59@srcf.ucam.org, pmatilai@redhat.com, jannh@google.com,
dhowells@redhat.com, jikos@kernel.org, mkoutny@suse.com,
ppavlu@suse.com, petr.vorel@gmail.com, mzerqung@0pointer.de,
kgold@linux.ibm.com, Roberto Sassu
In-Reply-To: <20241119104922.2772571-1-roberto.sassu@huaweicloud.com>
> On Nov 19, 2024, at 3:49 AM, Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> The Integrity Digest Cache can also help IMA for appraisal. IMA can simply
> lookup the calculated digest of an accessed file in the list of digests
> extracted from package headers, after verifying the header signature. It is
> sufficient to verify only one signature for all files in the package, as
> opposed to verifying a signature for each file.
Is there a way to maintain integrity over time? Today if a CVE is discovered
in a signed program, the program hash can be added to the blacklist keyring.
Later if IMA appraisal is used, the signature validation will fail just for that
program. With the Integrity Digest Cache, is there a way to do this?
^ permalink raw reply
* Re: [PATCH v6 07/15] digest_cache: Allow registration of digest list parsers
From: Luis Chamberlain @ 2024-11-25 23:53 UTC (permalink / raw)
To: Roberto Sassu
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, petr.pavlu,
samitolvanen, da.gomez, akpm, paul, jmorris, serge, shuah,
mcoquelin.stm32, alexandre.torgue, linux-integrity, linux-doc,
linux-kernel, linux-api, linux-modules, linux-security-module,
linux-kselftest, wufan, pbrobinson, zbyszek, hch, mjg59, pmatilai,
jannh, dhowells, jikos, mkoutny, ppavlu, petr.vorel, mzerqung,
kgold, Roberto Sassu
In-Reply-To: <20241119104922.2772571-8-roberto.sassu@huaweicloud.com>
On Tue, Nov 19, 2024 at 11:49:14AM +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> Introduce load_parser() to load a kernel module containing a
> parser for the requested digest list format (compressed kernel modules are
> supported). Kernel modules are searched in the
> /lib/modules/<kernel ver>/security/integrity/digest_cache directory.
>
> load_parser() calls ksys_finit_module() to load a kernel module directly
> from the kernel. request_module() cannot be used at this point, since the
> reference digests of modprobe and the linked libraries (required for IMA
> appraisal) might not be yet available, resulting in modprobe execution
> being denied.
You are doing a full solution implementation of loading modules in-kernel.
Appraisals of modules is just part of the boot process, some module
loading may need firmware to loading to get some functinality to work
for example some firmware to get a network device up or a GPU driver.
So module loading alone is not the only thing which may require
IMA appraisal, and this solution only addresses modules. There are other
things which may be needed other than firmware, eBPF programs are
another example.
It sounds more like you want to provide or extend LSM hooks fit your
architecture and make kernel_read_file() LSM hooks optionally use it to
fit this model.
Because this is just for a *phase* in boot, which you've caught because
a catch-22 situaton, where you didn't have your parsers loaded. Which is
just a reflection that you hit that snag. It doesn't prove all snags
will be caught yet.
And you only want to rely on this .. in-kernel loading solution only
early on boot, is there a way to change this over to enable regular
operation later?
Luis
^ permalink raw reply
* Re: [PATCH v6 02/15] module: Introduce ksys_finit_module()
From: Luis Chamberlain @ 2024-11-25 23:40 UTC (permalink / raw)
To: Roberto Sassu
Cc: Christoph Hellwig, zohar, dmitry.kasatkin, eric.snowberg, corbet,
petr.pavlu, samitolvanen, da.gomez, akpm, paul, jmorris, serge,
shuah, mcoquelin.stm32, alexandre.torgue, linux-integrity,
linux-doc, linux-kernel, linux-api, linux-modules,
linux-security-module, linux-kselftest, wufan, pbrobinson,
zbyszek, mjg59, pmatilai, jannh, dhowells, jikos, mkoutny, ppavlu,
petr.vorel, mzerqung, kgold, Roberto Sassu
In-Reply-To: <70952351d25817211509bf1cf43d3e665aef1481.camel@huaweicloud.com>
On Wed, Nov 20, 2024 at 10:16:23AM +0100, Roberto Sassu wrote:
> Again, maybe I misunderstood, but I'm not introducing any functional
> change to the current behavior, the kernel side also provides a file
> descriptor and module arguments as user space would do (e.g. by
> executing insmod).
The commit log does an extremely poor job to make this clear, all you
are doing here is adding a helper. The commit log should be super clear
on that and it is not.
Luis
^ permalink raw reply
* Re: [PATCH v21 1/6] exec: Add a new AT_EXECVE_CHECK flag to execveat(2)
From: Jeff Xu @ 2024-11-25 17:38 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Jeff Xu, Al Viro, Christian Brauner, Kees Cook, Paul Moore,
Serge Hallyn, Adhemerval Zanella Netto, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Elliott Hughes,
Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Linus Torvalds, Luca Boccassi, Luis Chamberlain,
Madhavan T . Venkataraman, Matt Bobrowski, Matthew Garrett,
Matthew Wilcox, Miklos Szeredi, Mimi Zohar, Nicolas Bouchinet,
Scott Shell, Shuah Khan, Stephen Rothwell, Steve Dower,
Steve Grubb, Theodore Ts'o, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Eric Paris, audit
In-Reply-To: <20241122.akooL5pie0th@digikod.net>
On Fri, Nov 22, 2024 at 6:50 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> On Thu, Nov 21, 2024 at 10:27:40AM -0800, Jeff Xu wrote:
> > On Thu, Nov 21, 2024 at 5:40 AM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > On Wed, Nov 20, 2024 at 08:06:07AM -0800, Jeff Xu wrote:
> > > > On Wed, Nov 20, 2024 at 1:42 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > >
> > > > > On Tue, Nov 19, 2024 at 05:17:00PM -0800, Jeff Xu wrote:
> > > > > > On Tue, Nov 12, 2024 at 11:22 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > > > >
> > > > > > > Add a new AT_EXECVE_CHECK flag to execveat(2) to check if a file would
> > > > > > > be allowed for execution. The main use case is for script interpreters
> > > > > > > and dynamic linkers to check execution permission according to the
> > > > > > > kernel's security policy. Another use case is to add context to access
> > > > > > > logs e.g., which script (instead of interpreter) accessed a file. As
> > > > > > > any executable code, scripts could also use this check [1].
> > > > > > >
> > > > > > > This is different from faccessat(2) + X_OK which only checks a subset of
> > > > > > > access rights (i.e. inode permission and mount options for regular
> > > > > > > files), but not the full context (e.g. all LSM access checks). The main
> > > > > > > use case for access(2) is for SUID processes to (partially) check access
> > > > > > > on behalf of their caller. The main use case for execveat(2) +
> > > > > > > AT_EXECVE_CHECK is to check if a script execution would be allowed,
> > > > > > > according to all the different restrictions in place. Because the use
> > > > > > > of AT_EXECVE_CHECK follows the exact kernel semantic as for a real
> > > > > > > execution, user space gets the same error codes.
> > > > > > >
> > > > > > > An interesting point of using execveat(2) instead of openat2(2) is that
> > > > > > > it decouples the check from the enforcement. Indeed, the security check
> > > > > > > can be logged (e.g. with audit) without blocking an execution
> > > > > > > environment not yet ready to enforce a strict security policy.
> > > > > > >
> > > > > > > LSMs can control or log execution requests with
> > > > > > > security_bprm_creds_for_exec(). However, to enforce a consistent and
> > > > > > > complete access control (e.g. on binary's dependencies) LSMs should
> > > > > > > restrict file executability, or mesure executed files, with
> > > > > > > security_file_open() by checking file->f_flags & __FMODE_EXEC.
> > > > > > >
> > > > > > > Because AT_EXECVE_CHECK is dedicated to user space interpreters, it
> > > > > > > doesn't make sense for the kernel to parse the checked files, look for
> > > > > > > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > > > > > > if the format is unknown. Because of that, security_bprm_check() is
> > > > > > > never called when AT_EXECVE_CHECK is used.
> > > > > > >
> > > > > > > It should be noted that script interpreters cannot directly use
> > > > > > > execveat(2) (without this new AT_EXECVE_CHECK flag) because this could
> > > > > > > lead to unexpected behaviors e.g., `python script.sh` could lead to Bash
> > > > > > > being executed to interpret the script. Unlike the kernel, script
> > > > > > > interpreters may just interpret the shebang as a simple comment, which
> > > > > > > should not change for backward compatibility reasons.
> > > > > > >
> > > > > > > Because scripts or libraries files might not currently have the
> > > > > > > executable permission set, or because we might want specific users to be
> > > > > > > allowed to run arbitrary scripts, the following patch provides a dynamic
> > > > > > > configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> > > > > > > SECBIT_EXEC_DENY_INTERACTIVE securebits.
> > > > > > >
> > > > > > > This is a redesign of the CLIP OS 4's O_MAYEXEC:
> > > > > > > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > > > > > > This patch has been used for more than a decade with customized script
> > > > > > > interpreters. Some examples can be found here:
> > > > > > > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> > > > > > >
> > > > > > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > > > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > > > > Cc: Kees Cook <keescook@chromium.org>
> > > > > > > Cc: Paul Moore <paul@paul-moore.com>
> > > > > > > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> > > > > > > Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> > > > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > > > > Link: https://lore.kernel.org/r/20241112191858.162021-2-mic@digikod.net
> > > > > > > ---
> > > > > > >
> > > > > > > Changes since v20:
> > > > > > > * Rename AT_CHECK to AT_EXECVE_CHECK, requested by Amir Goldstein and
> > > > > > > Serge Hallyn.
> > > > > > > * Move the UAPI documentation to a dedicated RST file.
> > > > > > > * Add Reviewed-by: Serge Hallyn
> > > > > > >
> > > > > > > Changes since v19:
> > > > > > > * Remove mention of "role transition" as suggested by Andy.
> > > > > > > * Highlight the difference between security_bprm_creds_for_exec() and
> > > > > > > the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> > > > > > > discussed with Jeff.
> > > > > > > * Improve documentation both in UAPI comments and kernel comments
> > > > > > > (requested by Kees).
> > > > > > >
> > > > > > > New design since v18:
> > > > > > > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > > > > > > ---
> > > > > > > Documentation/userspace-api/check_exec.rst | 34 ++++++++++++++++++++++
> > > > > > > Documentation/userspace-api/index.rst | 1 +
> > > > > > > fs/exec.c | 20 +++++++++++--
> > > > > > > include/linux/binfmts.h | 7 ++++-
> > > > > > > include/uapi/linux/fcntl.h | 4 +++
> > > > > > > kernel/audit.h | 1 +
> > > > > > > kernel/auditsc.c | 1 +
> > > > > > > security/security.c | 10 +++++++
> > > > > > > 8 files changed, 75 insertions(+), 3 deletions(-)
> > > > > > > create mode 100644 Documentation/userspace-api/check_exec.rst
> > > > > > >
> > > > > > > diff --git a/Documentation/userspace-api/check_exec.rst b/Documentation/userspace-api/check_exec.rst
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..ad1aeaa5f6c0
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/userspace-api/check_exec.rst
> > > > > > > @@ -0,0 +1,34 @@
> > > > > > > +===================
> > > > > > > +Executability check
> > > > > > > +===================
> > > > > > > +
> > > > > > > +AT_EXECVE_CHECK
> > > > > > > +===============
> > > > > > > +
> > > > > > > +Passing the ``AT_EXECVE_CHECK`` flag to :manpage:`execveat(2)` only performs a
> > > > > > > +check on a regular file and returns 0 if execution of this file would be
> > > > > > > +allowed, ignoring the file format and then the related interpreter dependencies
> > > > > > > +(e.g. ELF libraries, script's shebang).
> > > > > > > +
> > > > > > > +Programs should always perform this check to apply kernel-level checks against
> > > > > > > +files that are not directly executed by the kernel but passed to a user space
> > > > > > > +interpreter instead. All files that contain executable code, from the point of
> > > > > > > +view of the interpreter, should be checked. However the result of this check
> > > > > > > +should only be enforced according to ``SECBIT_EXEC_RESTRICT_FILE`` or
> > > > > > > +``SECBIT_EXEC_DENY_INTERACTIVE.``.
> > > > > > Regarding "should only"
> > > > > > Userspace (e.g. libc) could decide to enforce even when
> > > > > > SECBIT_EXEC_RESTRICT_FILE=0), i.e. if it determines not-enforcing
> > > > > > doesn't make sense.
> > > > >
> > > > > User space is always in control, but I don't think it would be wise to
> > > > > not follow the configuration securebits (in a generic system) because
> > > > > this could result to unattended behaviors (I don't have a specific one
> > > > > in mind but...). That being said, configuration and checks are
> > > > > standalones and specific/tailored systems are free to do the checks they
> > > > > want.
> > > > >
> > > > In the case of dynamic linker, we can always enforce honoring the
> > > > execveat(AT_EXECVE_CHECK) result, right ? I can't think of a case not
> > > > to, the dynamic linker doesn't need to check the
> > > > SECBIT_EXEC_RESTRICT_FILE bit.
> > >
> > > If the library file is not allowed to be executed by *all* access
> > > control systems (not just mount and file permission, but all LSMs), then
> > > the AT_EXECVE_CHECK will fail, which is OK as long as it is not a hard
> > > requirement.
> > Yes. specifically for the library loading case, I can't think of a
> > case where we need to by-pass LSMs. (letting user space to by-pass
> > LSM check seems questionable in concept, and should only be used when
> > there aren't other solutions). In the context of SELINUX enforcing
> > mode, we will want to enforce it. In the context of process level LSM
> > such as landlock, the process can already decide for itself by
> > selecting the policy for its own domain, it is unnecessary to use
> > another opt-out solution.
>
> My answer wasn't clear. The execveat(AT_EXECVE_CHECK) can and should
> always be done, but user space should only enforce restrictions
> according to the securebits.
>
I knew this part (AT_EXESCVE_CHECK is called always)
Since the securebits are enforced by userspace, setting it to 0 is
equivalent to opt-out enforcement, that is what I meant by opt-out.
> It doesn't make sense to talk about user space "bypassing" kernel
> checks. This patch series provides a feature to enable user space to
> enforce (at its level) the same checks as the kernel.
>
> There is no opt-out solution, but compatibility configuration bits
> through securebits (which can also be set by LSMs).
>
> To answer your question about the dynamic linker, there should be no
> difference of behavior with a script interpreter. Both should check
> executability but only enforce restriction according to the securebits
> (as explained in the documentation). Doing otherwise on a generic
> distro could lead to unexpected behaviors (e.g. if a user enforced a
> specific SELinux policy that doesn't allow execution of library files).
>
> >
> > There is one case where I see a difference:
> > ld.so a.out (when a.out is on non-exec mount)
> >
> > If the dynamic linker doesn't read SECBIT_EXEC_RESTRICT_FILE setting,
> > above will always fail. But that is more of a bugfix.
>
> No, the dynamic linker should only enforce restrictions according to the
> securebits, otherwise a user space update (e.g. with a new dynamic
> linker ignoring the securebits) could break an existing system.
>
OK. upgrade is a valid concern. Previously, I was just thinking about
a new LSM based on this check, not existing LSM policies.
Do you happen to know which SELinux policy/LSM could break ? i.e. it
will be applied to libraries once we add AT_EXESCVE_CHECK in the
dynamic linker.
We could give heads up and prepare for that.
> >
> > >Relying on the securebits to know if this is a hard
> > > requirement or not enables system administrator and distros to control
> > > this potential behavior change.
> > >
> > I think, for the dynamic linker, it can be a hard requirement.
>
> Not on a generic distro.
>
Ok. Maybe this can be done through a configuration option for the
dynamic linker.
The consideration I have is: securebits is currently designed to
control both dynamic linker and shell scripts.
The case for dynamic linker is simpler than scripts cases, (non-exec
mount, and perhaps some LSM policies for libraries) and distributions
such as ChromeOS can enforce the dynamic linker case ahead of scripts
interrupter cases, i.e. without waiting for python/shell being
upgraded, that can take sometimes.
> >
> > For scripts, the cases are more complicated and we can't just enforce
> > it, therefore have to rely on security bits to give a pre-process
> > level control.
> >
> > > >
> > > > script interpreters need to check this though, because the apps might
> > > > need to adjust/test the scripts they are calling, so
> > > > SECBIT_EXEC_RESTRICT_FILE can be used to opt-out the enforcement.
> > > >
> > > > > > When SECBIT_EXEC_RESTRICT_FILE=1, userspace is bound to enforce.
> > > > > >
> > > > > > > +
> > > > > > > +The main purpose of this flag is to improve the security and consistency of an
> > > > > > > +execution environment to ensure that direct file execution (e.g.
> > > > > > > +``./script.sh``) and indirect file execution (e.g. ``sh script.sh``) lead to
> > > > > > > +the same result. For instance, this can be used to check if a file is
> > > > > > > +trustworthy according to the caller's environment.
> > > > > > > +
> > > > > > > +In a secure environment, libraries and any executable dependencies should also
> > > > > > > +be checked. For instance, dynamic linking should make sure that all libraries
> > > > > > > +are allowed for execution to avoid trivial bypass (e.g. using ``LD_PRELOAD``).
> > > > > > > +For such secure execution environment to make sense, only trusted code should
> > > > > > > +be executable, which also requires integrity guarantees.
> > > > > > > +
> > > > > > > +To avoid race conditions leading to time-of-check to time-of-use issues,
> > > > > > > +``AT_EXECVE_CHECK`` should be used with ``AT_EMPTY_PATH`` to check against a
> > > > > > > +file descriptor instead of a path.
> > > > > > > diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
> > > > > > > index 274cc7546efc..6272bcf11296 100644
> > > > > > > --- a/Documentation/userspace-api/index.rst
> > > > > > > +++ b/Documentation/userspace-api/index.rst
> > > > > > > @@ -35,6 +35,7 @@ Security-related interfaces
> > > > > > > mfd_noexec
> > > > > > > spec_ctrl
> > > > > > > tee
> > > > > > > + check_exec
> > > > > > >
> > > > > > > Devices and I/O
> > > > > > > ===============
> > > > > > > diff --git a/fs/exec.c b/fs/exec.c
> > > > > > > index 6c53920795c2..bb83b6a39530 100644
> > > > > > > --- a/fs/exec.c
> > > > > > > +++ b/fs/exec.c
> > > > > > > @@ -891,7 +891,8 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > > > > > > .lookup_flags = LOOKUP_FOLLOW,
> > > > > > > };
> > > > > > >
> > > > > > > - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > > > > > > + if ((flags &
> > > > > > > + ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_EXECVE_CHECK)) != 0)
> > > > > > > return ERR_PTR(-EINVAL);
> > > > > > > if (flags & AT_SYMLINK_NOFOLLOW)
> > > > > > > open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> > > > > > > @@ -1545,6 +1546,21 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
> > > > > > > }
> > > > > > > bprm->interp = bprm->filename;
> > > > > > >
> > > > > > > + /*
> > > > > > > + * At this point, security_file_open() has already been called (with
> > > > > > > + * __FMODE_EXEC) and access control checks for AT_EXECVE_CHECK will
> > > > > > > + * stop just after the security_bprm_creds_for_exec() call in
> > > > > > > + * bprm_execve(). Indeed, the kernel should not try to parse the
> > > > > > > + * content of the file with exec_binprm() nor change the calling
> > > > > > > + * thread, which means that the following security functions will be
> > > > > > > + * not called:
> > > > > > > + * - security_bprm_check()
> > > > > > > + * - security_bprm_creds_from_file()
> > > > > > > + * - security_bprm_committing_creds()
> > > > > > > + * - security_bprm_committed_creds()
> > > > > > > + */
> > > > > > > + bprm->is_check = !!(flags & AT_EXECVE_CHECK);
> > > > > > > +
> > > > > > > retval = bprm_mm_init(bprm);
> > > > > > > if (!retval)
> > > > > > > return bprm;
> > > > > > > @@ -1839,7 +1855,7 @@ static int bprm_execve(struct linux_binprm *bprm)
> > > > > > >
> > > > > > > /* Set the unchanging part of bprm->cred */
> > > > > > > retval = security_bprm_creds_for_exec(bprm);
> > > > > > > - if (retval)
> > > > > > > + if (retval || bprm->is_check)
> > > > > > > goto out;
> > > > > > >
> > > > > > > retval = exec_binprm(bprm);
> > > > > > > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> > > > > > > index e6c00e860951..8ff0eb3644a1 100644
> > > > > > > --- a/include/linux/binfmts.h
> > > > > > > +++ b/include/linux/binfmts.h
> > > > > > > @@ -42,7 +42,12 @@ struct linux_binprm {
> > > > > > > * Set when errors can no longer be returned to the
> > > > > > > * original userspace.
> > > > > > > */
> > > > > > > - point_of_no_return:1;
> > > > > > > + point_of_no_return:1,
> > > > > > > + /*
> > > > > > > + * Set by user space to check executability according to the
> > > > > > > + * caller's environment.
> > > > > > > + */
> > > > > > > + is_check:1;
> > > > > > > struct file *executable; /* Executable to pass to the interpreter */
> > > > > > > struct file *interpreter;
> > > > > > > struct file *file;
> > > > > > > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > > > > > > index 87e2dec79fea..2e87f2e3a79f 100644
> > > > > > > --- a/include/uapi/linux/fcntl.h
> > > > > > > +++ b/include/uapi/linux/fcntl.h
> > > > > > > @@ -154,6 +154,10 @@
> > > > > > > usable with open_by_handle_at(2). */
> > > > > > > #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
> > > > > > >
> > > > > > > +/* Flags for execveat2(2). */
> > > > > > > +#define AT_EXECVE_CHECK 0x10000 /* Only perform a check if execution
> > > > > > > + would be allowed. */
> > > > > > > +
> > > > > > > #if defined(__KERNEL__)
> > > > > > > #define AT_GETATTR_NOSEC 0x80000000
> > > > > > > #endif
> > > > > > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > > > > > index a60d2840559e..8ebdabd2ab81 100644
> > > > > > > --- a/kernel/audit.h
> > > > > > > +++ b/kernel/audit.h
> > > > > > > @@ -197,6 +197,7 @@ struct audit_context {
> > > > > > > struct open_how openat2;
> > > > > > > struct {
> > > > > > > int argc;
> > > > > > > + bool is_check;
> > > > > > > } execve;
> > > > > > > struct {
> > > > > > > char *name;
> > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > > index cd57053b4a69..8d9ba5600cf2 100644
> > > > > > > --- a/kernel/auditsc.c
> > > > > > > +++ b/kernel/auditsc.c
> > > > > > > @@ -2662,6 +2662,7 @@ void __audit_bprm(struct linux_binprm *bprm)
> > > > > > >
> > > > > > > context->type = AUDIT_EXECVE;
> > > > > > > context->execve.argc = bprm->argc;
> > > > > > > + context->execve.is_check = bprm->is_check;
> > > > > > Where is execve.is_check used ?
> > > > >
> > > > > It is used in bprm_execve(), exposed to the audit framework, and
> > > > > potentially used by LSMs.
> > > > >
> > > > bprm_execve() uses bprm->is_check, not the context->execve.is_check.
> > >
> > > Correct, this is only for audit but not used yet.
> > >
> > > Paul, Eric, do you want me to remove this field, leave it, or extend
> > > this patch like this?
> > >
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index 8d9ba5600cf2..12cf89fa224a 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -1290,6 +1290,8 @@ static void audit_log_execve_info(struct audit_context *context,
> > > }
> > > } while (arg < context->execve.argc);
> > >
> > > + audit_log_format(*ab, " check=%d", context->execve.is_check);
> > > +
> > > /* NOTE: the caller handles the final audit_log_end() call */
> > >
> > > out:
> > >
> > > >
> > > >
> > > > > >
> > > > > >
> > > > > > > }
> > > > > > >
> > > > > > >
> > > > > > > diff --git a/security/security.c b/security/security.c
> > > > > > > index c5981e558bc2..456361ec249d 100644
> > > > > > > --- a/security/security.c
> > > > > > > +++ b/security/security.c
> > > > > > > @@ -1249,6 +1249,12 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> > > > > > > * to 1 if AT_SECURE should be set to request libc enable secure mode. @bprm
> > > > > > > * contains the linux_binprm structure.
> > > > > > > *
> > > > > > > + * If execveat(2) is called with the AT_EXECVE_CHECK flag, bprm->is_check is
> > > > > > > + * set. The result must be the same as without this flag even if the execution
> > > > > > > + * will never really happen and @bprm will always be dropped.
> > > > > > > + *
> > > > > > > + * This hook must not change current->cred, only @bprm->cred.
> > > > > > > + *
> > > > > > > * Return: Returns 0 if the hook is successful and permission is granted.
> > > > > > > */
> > > > > > > int security_bprm_creds_for_exec(struct linux_binprm *bprm)
> > > > > > > @@ -3100,6 +3106,10 @@ int security_file_receive(struct file *file)
> > > > > > > * Save open-time permission checking state for later use upon file_permission,
> > > > > > > * and recheck access if anything has changed since inode_permission.
> > > > > > > *
> > > > > > > + * We can check if a file is opened for execution (e.g. execve(2) call), either
> > > > > > > + * directly or indirectly (e.g. ELF's ld.so) by checking file->f_flags &
> > > > > > > + * __FMODE_EXEC .
> > > > > > > + *
> > > > > > > * Return: Returns 0 if permission is granted.
> > > > > > > */
> > > > > > > int security_file_open(struct file *file)
> > > > > > > --
> > > > > > > 2.47.0
> > > > > > >
> > > > > > >
> > > >
^ permalink raw reply
* Re: [PATCH v2] vfs: add RWF_NOAPPEND flag for pwritev2
From: John Garry @ 2024-11-25 14:59 UTC (permalink / raw)
To: Alejandro Colomar
Cc: asml.silence, dalias, jannh, linux-api, linux-fsdevel,
linux-kernel, viro
In-Reply-To: <Z0RTHYtaqEoffNrG@devuan>
On 25/11/2024 10:36, Alejandro Colomar wrote:
> Hi,
>
> Here's a gentle ping, as John reminded me that there's no documentation
> for this in the manual pages. Would anyone want to send a patch?
>
> Have a lovely day!
> Alex
>
>
Nobody was forthcoming previously, so I will just send something. I am
not too familiar with that flag, though...
John
^ permalink raw reply
* Re: [PATCH v2] vfs: add RWF_NOAPPEND flag for pwritev2
From: Alejandro Colomar @ 2024-11-25 10:36 UTC (permalink / raw)
To: john.g.garry
Cc: asml.silence, dalias, jannh, linux-api, linux-fsdevel,
linux-kernel, viro
In-Reply-To: <f20a786f-156a-4772-8633-66518bd09a02@oracle.com>
[-- Attachment #1: Type: text/plain, Size: 221 bytes --]
Hi,
Here's a gentle ping, as John reminded me that there's no documentation
for this in the manual pages. Would anyone want to send a patch?
Have a lovely day!
Alex
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v2 00/12] Begin upstreaming Homa transport protocol
From: Cong Wang @ 2024-11-22 21:42 UTC (permalink / raw)
To: John Ousterhout; +Cc: netdev, linux-api
In-Reply-To: <CAGXJAmwr5u7Don-nKivvTuj6C9pGT2A5cAXvhKo6sAm_hHy72A@mail.gmail.com>
On Tue, Nov 19, 2024 at 01:13:36PM -0800, John Ousterhout wrote:
> On Thu, Nov 14, 2024 at 8:59 AM John Ousterhout <ouster@cs.stanford.edu> wrote:
> >
> > On Wed, Nov 13, 2024 at 9:08 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > > 2. Please consider adding socket diagnostics, see net/ipv4/inet_diag.c.
> >
> > I wasn't familiar with them before your email; I'll take a look.
>
> I have taken a look at socket diagnostics, and I agree that Homa
> should support them. However, this looks like a nontrivial task: there
> appears to be a fair amount of complexity there, not much
> documentation, and the current diagnostics appear pretty TCP-specific
> so it may take some work (and advice) to figure out how to map them
> onto Homa. Thus it feels like it will take a fair amount of
> back-and-forth to get this right. I would prefer to defer this until
> after the main body of Homa has been upstreamed, then have a patch
> series that is dedicated to socket diagnostics support, rather than
> lumping socket diagnostics into the current patch series. Is that OK
> with you?
>
Sure, I don't think it is a blocker here, it is just a nice feature to
promote Homa.
Thanks.
^ permalink raw reply
* Re: [PATCH v21 6/6] samples/check-exec: Add an enlighten "inc" interpreter and 28 tests
From: Mickaël Salaün @ 2024-11-22 14:50 UTC (permalink / raw)
To: Mimi Zohar
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Nicolas Bouchinet, Scott Shell, Shuah Khan, Stephen Rothwell,
Steve Dower, Steve Grubb, Theodore Ts'o, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <d115a20889d01bc7b12dbd8cf99aad0be58cbc97.camel@linux.ibm.com>
On Thu, Nov 21, 2024 at 03:34:47PM -0500, Mimi Zohar wrote:
> Hi Mickaël,
>
> On Tue, 2024-11-12 at 20:18 +0100, Mickaël Salaün wrote:
> >
> > +
> > +/* Returns 1 on error, 0 otherwise. */
> > +static int interpret_stream(FILE *script, char *const script_name,
> > + char *const *const envp, const bool restrict_stream)
> > +{
> > + int err;
> > + char *const script_argv[] = { script_name, NULL };
> > + char buf[128] = {};
> > + size_t buf_size = sizeof(buf);
> > +
> > + /*
> > + * We pass a valid argv and envp to the kernel to emulate a native
> > + * script execution. We must use the script file descriptor instead of
> > + * the script path name to avoid race conditions.
> > + */
> > + err = execveat(fileno(script), "", script_argv, envp,
> > + AT_EMPTY_PATH | AT_EXECVE_CHECK);
>
> At least with v20, the AT_CHECK always was being set, independent of whether
> set-exec.c set it. I'll re-test with v21.
AT_EXECVE_CEHCK should always be set, only the interpretation of the
result should be relative to securebits. This is highlighted in the
documentation.
>
> thanks,
>
> Mimi
>
> > + if (err && restrict_stream) {
> > + perror("ERROR: Script execution check");
> > + return 1;
> > + }
> > +
> > + /* Reads script. */
> > + buf_size = fread(buf, 1, buf_size - 1, script);
> > + return interpret_buffer(buf, buf_size);
> > +}
> > +
>
>
^ permalink raw reply
* Re: [PATCH v21 1/6] exec: Add a new AT_EXECVE_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-11-22 14:50 UTC (permalink / raw)
To: Jeff Xu
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Theodore Ts'o,
Thibaut Sautereau, Vincent Strubel, Xiaoming Ni, Yin Fengwei,
kernel-hardening, linux-api, linux-fsdevel, linux-integrity,
linux-kernel, linux-security-module, Eric Paris, audit
In-Reply-To: <CABi2SkVHW9MBm=quZPdim_pM=BPaUD-8jRjG6G8OyQ8fQVsm0A@mail.gmail.com>
On Thu, Nov 21, 2024 at 10:27:40AM -0800, Jeff Xu wrote:
> On Thu, Nov 21, 2024 at 5:40 AM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > On Wed, Nov 20, 2024 at 08:06:07AM -0800, Jeff Xu wrote:
> > > On Wed, Nov 20, 2024 at 1:42 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > >
> > > > On Tue, Nov 19, 2024 at 05:17:00PM -0800, Jeff Xu wrote:
> > > > > On Tue, Nov 12, 2024 at 11:22 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > > >
> > > > > > Add a new AT_EXECVE_CHECK flag to execveat(2) to check if a file would
> > > > > > be allowed for execution. The main use case is for script interpreters
> > > > > > and dynamic linkers to check execution permission according to the
> > > > > > kernel's security policy. Another use case is to add context to access
> > > > > > logs e.g., which script (instead of interpreter) accessed a file. As
> > > > > > any executable code, scripts could also use this check [1].
> > > > > >
> > > > > > This is different from faccessat(2) + X_OK which only checks a subset of
> > > > > > access rights (i.e. inode permission and mount options for regular
> > > > > > files), but not the full context (e.g. all LSM access checks). The main
> > > > > > use case for access(2) is for SUID processes to (partially) check access
> > > > > > on behalf of their caller. The main use case for execveat(2) +
> > > > > > AT_EXECVE_CHECK is to check if a script execution would be allowed,
> > > > > > according to all the different restrictions in place. Because the use
> > > > > > of AT_EXECVE_CHECK follows the exact kernel semantic as for a real
> > > > > > execution, user space gets the same error codes.
> > > > > >
> > > > > > An interesting point of using execveat(2) instead of openat2(2) is that
> > > > > > it decouples the check from the enforcement. Indeed, the security check
> > > > > > can be logged (e.g. with audit) without blocking an execution
> > > > > > environment not yet ready to enforce a strict security policy.
> > > > > >
> > > > > > LSMs can control or log execution requests with
> > > > > > security_bprm_creds_for_exec(). However, to enforce a consistent and
> > > > > > complete access control (e.g. on binary's dependencies) LSMs should
> > > > > > restrict file executability, or mesure executed files, with
> > > > > > security_file_open() by checking file->f_flags & __FMODE_EXEC.
> > > > > >
> > > > > > Because AT_EXECVE_CHECK is dedicated to user space interpreters, it
> > > > > > doesn't make sense for the kernel to parse the checked files, look for
> > > > > > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > > > > > if the format is unknown. Because of that, security_bprm_check() is
> > > > > > never called when AT_EXECVE_CHECK is used.
> > > > > >
> > > > > > It should be noted that script interpreters cannot directly use
> > > > > > execveat(2) (without this new AT_EXECVE_CHECK flag) because this could
> > > > > > lead to unexpected behaviors e.g., `python script.sh` could lead to Bash
> > > > > > being executed to interpret the script. Unlike the kernel, script
> > > > > > interpreters may just interpret the shebang as a simple comment, which
> > > > > > should not change for backward compatibility reasons.
> > > > > >
> > > > > > Because scripts or libraries files might not currently have the
> > > > > > executable permission set, or because we might want specific users to be
> > > > > > allowed to run arbitrary scripts, the following patch provides a dynamic
> > > > > > configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> > > > > > SECBIT_EXEC_DENY_INTERACTIVE securebits.
> > > > > >
> > > > > > This is a redesign of the CLIP OS 4's O_MAYEXEC:
> > > > > > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > > > > > This patch has been used for more than a decade with customized script
> > > > > > interpreters. Some examples can be found here:
> > > > > > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> > > > > >
> > > > > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > > > Cc: Kees Cook <keescook@chromium.org>
> > > > > > Cc: Paul Moore <paul@paul-moore.com>
> > > > > > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> > > > > > Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> > > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > > > Link: https://lore.kernel.org/r/20241112191858.162021-2-mic@digikod.net
> > > > > > ---
> > > > > >
> > > > > > Changes since v20:
> > > > > > * Rename AT_CHECK to AT_EXECVE_CHECK, requested by Amir Goldstein and
> > > > > > Serge Hallyn.
> > > > > > * Move the UAPI documentation to a dedicated RST file.
> > > > > > * Add Reviewed-by: Serge Hallyn
> > > > > >
> > > > > > Changes since v19:
> > > > > > * Remove mention of "role transition" as suggested by Andy.
> > > > > > * Highlight the difference between security_bprm_creds_for_exec() and
> > > > > > the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> > > > > > discussed with Jeff.
> > > > > > * Improve documentation both in UAPI comments and kernel comments
> > > > > > (requested by Kees).
> > > > > >
> > > > > > New design since v18:
> > > > > > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > > > > > ---
> > > > > > Documentation/userspace-api/check_exec.rst | 34 ++++++++++++++++++++++
> > > > > > Documentation/userspace-api/index.rst | 1 +
> > > > > > fs/exec.c | 20 +++++++++++--
> > > > > > include/linux/binfmts.h | 7 ++++-
> > > > > > include/uapi/linux/fcntl.h | 4 +++
> > > > > > kernel/audit.h | 1 +
> > > > > > kernel/auditsc.c | 1 +
> > > > > > security/security.c | 10 +++++++
> > > > > > 8 files changed, 75 insertions(+), 3 deletions(-)
> > > > > > create mode 100644 Documentation/userspace-api/check_exec.rst
> > > > > >
> > > > > > diff --git a/Documentation/userspace-api/check_exec.rst b/Documentation/userspace-api/check_exec.rst
> > > > > > new file mode 100644
> > > > > > index 000000000000..ad1aeaa5f6c0
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/userspace-api/check_exec.rst
> > > > > > @@ -0,0 +1,34 @@
> > > > > > +===================
> > > > > > +Executability check
> > > > > > +===================
> > > > > > +
> > > > > > +AT_EXECVE_CHECK
> > > > > > +===============
> > > > > > +
> > > > > > +Passing the ``AT_EXECVE_CHECK`` flag to :manpage:`execveat(2)` only performs a
> > > > > > +check on a regular file and returns 0 if execution of this file would be
> > > > > > +allowed, ignoring the file format and then the related interpreter dependencies
> > > > > > +(e.g. ELF libraries, script's shebang).
> > > > > > +
> > > > > > +Programs should always perform this check to apply kernel-level checks against
> > > > > > +files that are not directly executed by the kernel but passed to a user space
> > > > > > +interpreter instead. All files that contain executable code, from the point of
> > > > > > +view of the interpreter, should be checked. However the result of this check
> > > > > > +should only be enforced according to ``SECBIT_EXEC_RESTRICT_FILE`` or
> > > > > > +``SECBIT_EXEC_DENY_INTERACTIVE.``.
> > > > > Regarding "should only"
> > > > > Userspace (e.g. libc) could decide to enforce even when
> > > > > SECBIT_EXEC_RESTRICT_FILE=0), i.e. if it determines not-enforcing
> > > > > doesn't make sense.
> > > >
> > > > User space is always in control, but I don't think it would be wise to
> > > > not follow the configuration securebits (in a generic system) because
> > > > this could result to unattended behaviors (I don't have a specific one
> > > > in mind but...). That being said, configuration and checks are
> > > > standalones and specific/tailored systems are free to do the checks they
> > > > want.
> > > >
> > > In the case of dynamic linker, we can always enforce honoring the
> > > execveat(AT_EXECVE_CHECK) result, right ? I can't think of a case not
> > > to, the dynamic linker doesn't need to check the
> > > SECBIT_EXEC_RESTRICT_FILE bit.
> >
> > If the library file is not allowed to be executed by *all* access
> > control systems (not just mount and file permission, but all LSMs), then
> > the AT_EXECVE_CHECK will fail, which is OK as long as it is not a hard
> > requirement.
> Yes. specifically for the library loading case, I can't think of a
> case where we need to by-pass LSMs. (letting user space to by-pass
> LSM check seems questionable in concept, and should only be used when
> there aren't other solutions). In the context of SELINUX enforcing
> mode, we will want to enforce it. In the context of process level LSM
> such as landlock, the process can already decide for itself by
> selecting the policy for its own domain, it is unnecessary to use
> another opt-out solution.
My answer wasn't clear. The execveat(AT_EXECVE_CHECK) can and should
always be done, but user space should only enforce restrictions
according to the securebits.
It doesn't make sense to talk about user space "bypassing" kernel
checks. This patch series provides a feature to enable user space to
enforce (at its level) the same checks as the kernel.
There is no opt-out solution, but compatibility configuration bits
through securebits (which can also be set by LSMs).
To answer your question about the dynamic linker, there should be no
difference of behavior with a script interpreter. Both should check
executability but only enforce restriction according to the securebits
(as explained in the documentation). Doing otherwise on a generic
distro could lead to unexpected behaviors (e.g. if a user enforced a
specific SELinux policy that doesn't allow execution of library files).
>
> There is one case where I see a difference:
> ld.so a.out (when a.out is on non-exec mount)
>
> If the dynamic linker doesn't read SECBIT_EXEC_RESTRICT_FILE setting,
> above will always fail. But that is more of a bugfix.
No, the dynamic linker should only enforce restrictions according to the
securebits, otherwise a user space update (e.g. with a new dynamic
linker ignoring the securebits) could break an existing system.
>
> >Relying on the securebits to know if this is a hard
> > requirement or not enables system administrator and distros to control
> > this potential behavior change.
> >
> I think, for the dynamic linker, it can be a hard requirement.
Not on a generic distro.
>
> For scripts, the cases are more complicated and we can't just enforce
> it, therefore have to rely on security bits to give a pre-process
> level control.
>
> > >
> > > script interpreters need to check this though, because the apps might
> > > need to adjust/test the scripts they are calling, so
> > > SECBIT_EXEC_RESTRICT_FILE can be used to opt-out the enforcement.
> > >
> > > > > When SECBIT_EXEC_RESTRICT_FILE=1, userspace is bound to enforce.
> > > > >
> > > > > > +
> > > > > > +The main purpose of this flag is to improve the security and consistency of an
> > > > > > +execution environment to ensure that direct file execution (e.g.
> > > > > > +``./script.sh``) and indirect file execution (e.g. ``sh script.sh``) lead to
> > > > > > +the same result. For instance, this can be used to check if a file is
> > > > > > +trustworthy according to the caller's environment.
> > > > > > +
> > > > > > +In a secure environment, libraries and any executable dependencies should also
> > > > > > +be checked. For instance, dynamic linking should make sure that all libraries
> > > > > > +are allowed for execution to avoid trivial bypass (e.g. using ``LD_PRELOAD``).
> > > > > > +For such secure execution environment to make sense, only trusted code should
> > > > > > +be executable, which also requires integrity guarantees.
> > > > > > +
> > > > > > +To avoid race conditions leading to time-of-check to time-of-use issues,
> > > > > > +``AT_EXECVE_CHECK`` should be used with ``AT_EMPTY_PATH`` to check against a
> > > > > > +file descriptor instead of a path.
> > > > > > diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
> > > > > > index 274cc7546efc..6272bcf11296 100644
> > > > > > --- a/Documentation/userspace-api/index.rst
> > > > > > +++ b/Documentation/userspace-api/index.rst
> > > > > > @@ -35,6 +35,7 @@ Security-related interfaces
> > > > > > mfd_noexec
> > > > > > spec_ctrl
> > > > > > tee
> > > > > > + check_exec
> > > > > >
> > > > > > Devices and I/O
> > > > > > ===============
> > > > > > diff --git a/fs/exec.c b/fs/exec.c
> > > > > > index 6c53920795c2..bb83b6a39530 100644
> > > > > > --- a/fs/exec.c
> > > > > > +++ b/fs/exec.c
> > > > > > @@ -891,7 +891,8 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > > > > > .lookup_flags = LOOKUP_FOLLOW,
> > > > > > };
> > > > > >
> > > > > > - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > > > > > + if ((flags &
> > > > > > + ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_EXECVE_CHECK)) != 0)
> > > > > > return ERR_PTR(-EINVAL);
> > > > > > if (flags & AT_SYMLINK_NOFOLLOW)
> > > > > > open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> > > > > > @@ -1545,6 +1546,21 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
> > > > > > }
> > > > > > bprm->interp = bprm->filename;
> > > > > >
> > > > > > + /*
> > > > > > + * At this point, security_file_open() has already been called (with
> > > > > > + * __FMODE_EXEC) and access control checks for AT_EXECVE_CHECK will
> > > > > > + * stop just after the security_bprm_creds_for_exec() call in
> > > > > > + * bprm_execve(). Indeed, the kernel should not try to parse the
> > > > > > + * content of the file with exec_binprm() nor change the calling
> > > > > > + * thread, which means that the following security functions will be
> > > > > > + * not called:
> > > > > > + * - security_bprm_check()
> > > > > > + * - security_bprm_creds_from_file()
> > > > > > + * - security_bprm_committing_creds()
> > > > > > + * - security_bprm_committed_creds()
> > > > > > + */
> > > > > > + bprm->is_check = !!(flags & AT_EXECVE_CHECK);
> > > > > > +
> > > > > > retval = bprm_mm_init(bprm);
> > > > > > if (!retval)
> > > > > > return bprm;
> > > > > > @@ -1839,7 +1855,7 @@ static int bprm_execve(struct linux_binprm *bprm)
> > > > > >
> > > > > > /* Set the unchanging part of bprm->cred */
> > > > > > retval = security_bprm_creds_for_exec(bprm);
> > > > > > - if (retval)
> > > > > > + if (retval || bprm->is_check)
> > > > > > goto out;
> > > > > >
> > > > > > retval = exec_binprm(bprm);
> > > > > > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> > > > > > index e6c00e860951..8ff0eb3644a1 100644
> > > > > > --- a/include/linux/binfmts.h
> > > > > > +++ b/include/linux/binfmts.h
> > > > > > @@ -42,7 +42,12 @@ struct linux_binprm {
> > > > > > * Set when errors can no longer be returned to the
> > > > > > * original userspace.
> > > > > > */
> > > > > > - point_of_no_return:1;
> > > > > > + point_of_no_return:1,
> > > > > > + /*
> > > > > > + * Set by user space to check executability according to the
> > > > > > + * caller's environment.
> > > > > > + */
> > > > > > + is_check:1;
> > > > > > struct file *executable; /* Executable to pass to the interpreter */
> > > > > > struct file *interpreter;
> > > > > > struct file *file;
> > > > > > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > > > > > index 87e2dec79fea..2e87f2e3a79f 100644
> > > > > > --- a/include/uapi/linux/fcntl.h
> > > > > > +++ b/include/uapi/linux/fcntl.h
> > > > > > @@ -154,6 +154,10 @@
> > > > > > usable with open_by_handle_at(2). */
> > > > > > #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
> > > > > >
> > > > > > +/* Flags for execveat2(2). */
> > > > > > +#define AT_EXECVE_CHECK 0x10000 /* Only perform a check if execution
> > > > > > + would be allowed. */
> > > > > > +
> > > > > > #if defined(__KERNEL__)
> > > > > > #define AT_GETATTR_NOSEC 0x80000000
> > > > > > #endif
> > > > > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > > > > index a60d2840559e..8ebdabd2ab81 100644
> > > > > > --- a/kernel/audit.h
> > > > > > +++ b/kernel/audit.h
> > > > > > @@ -197,6 +197,7 @@ struct audit_context {
> > > > > > struct open_how openat2;
> > > > > > struct {
> > > > > > int argc;
> > > > > > + bool is_check;
> > > > > > } execve;
> > > > > > struct {
> > > > > > char *name;
> > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > index cd57053b4a69..8d9ba5600cf2 100644
> > > > > > --- a/kernel/auditsc.c
> > > > > > +++ b/kernel/auditsc.c
> > > > > > @@ -2662,6 +2662,7 @@ void __audit_bprm(struct linux_binprm *bprm)
> > > > > >
> > > > > > context->type = AUDIT_EXECVE;
> > > > > > context->execve.argc = bprm->argc;
> > > > > > + context->execve.is_check = bprm->is_check;
> > > > > Where is execve.is_check used ?
> > > >
> > > > It is used in bprm_execve(), exposed to the audit framework, and
> > > > potentially used by LSMs.
> > > >
> > > bprm_execve() uses bprm->is_check, not the context->execve.is_check.
> >
> > Correct, this is only for audit but not used yet.
> >
> > Paul, Eric, do you want me to remove this field, leave it, or extend
> > this patch like this?
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 8d9ba5600cf2..12cf89fa224a 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1290,6 +1290,8 @@ static void audit_log_execve_info(struct audit_context *context,
> > }
> > } while (arg < context->execve.argc);
> >
> > + audit_log_format(*ab, " check=%d", context->execve.is_check);
> > +
> > /* NOTE: the caller handles the final audit_log_end() call */
> >
> > out:
> >
> > >
> > >
> > > > >
> > > > >
> > > > > > }
> > > > > >
> > > > > >
> > > > > > diff --git a/security/security.c b/security/security.c
> > > > > > index c5981e558bc2..456361ec249d 100644
> > > > > > --- a/security/security.c
> > > > > > +++ b/security/security.c
> > > > > > @@ -1249,6 +1249,12 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> > > > > > * to 1 if AT_SECURE should be set to request libc enable secure mode. @bprm
> > > > > > * contains the linux_binprm structure.
> > > > > > *
> > > > > > + * If execveat(2) is called with the AT_EXECVE_CHECK flag, bprm->is_check is
> > > > > > + * set. The result must be the same as without this flag even if the execution
> > > > > > + * will never really happen and @bprm will always be dropped.
> > > > > > + *
> > > > > > + * This hook must not change current->cred, only @bprm->cred.
> > > > > > + *
> > > > > > * Return: Returns 0 if the hook is successful and permission is granted.
> > > > > > */
> > > > > > int security_bprm_creds_for_exec(struct linux_binprm *bprm)
> > > > > > @@ -3100,6 +3106,10 @@ int security_file_receive(struct file *file)
> > > > > > * Save open-time permission checking state for later use upon file_permission,
> > > > > > * and recheck access if anything has changed since inode_permission.
> > > > > > *
> > > > > > + * We can check if a file is opened for execution (e.g. execve(2) call), either
> > > > > > + * directly or indirectly (e.g. ELF's ld.so) by checking file->f_flags &
> > > > > > + * __FMODE_EXEC .
> > > > > > + *
> > > > > > * Return: Returns 0 if permission is granted.
> > > > > > */
> > > > > > int security_file_open(struct file *file)
> > > > > > --
> > > > > > 2.47.0
> > > > > >
> > > > > >
> > >
^ permalink raw reply
* Re: [PATCH v21 6/6] samples/check-exec: Add an enlighten "inc" interpreter and 28 tests
From: Mimi Zohar @ 2024-11-21 20:34 UTC (permalink / raw)
To: Mickaël Salaün, Al Viro, Christian Brauner, Kees Cook,
Paul Moore, Serge Hallyn
Cc: Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Nicolas Bouchinet, Scott Shell, Shuah Khan, Stephen Rothwell,
Steve Dower, Steve Grubb, Theodore Ts'o, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241112191858.162021-7-mic@digikod.net>
Hi Mickaël,
On Tue, 2024-11-12 at 20:18 +0100, Mickaël Salaün wrote:
>
> +
> +/* Returns 1 on error, 0 otherwise. */
> +static int interpret_stream(FILE *script, char *const script_name,
> + char *const *const envp, const bool restrict_stream)
> +{
> + int err;
> + char *const script_argv[] = { script_name, NULL };
> + char buf[128] = {};
> + size_t buf_size = sizeof(buf);
> +
> + /*
> + * We pass a valid argv and envp to the kernel to emulate a native
> + * script execution. We must use the script file descriptor instead of
> + * the script path name to avoid race conditions.
> + */
> + err = execveat(fileno(script), "", script_argv, envp,
> + AT_EMPTY_PATH | AT_EXECVE_CHECK);
At least with v20, the AT_CHECK always was being set, independent of whether
set-exec.c set it. I'll re-test with v21.
thanks,
Mimi
> + if (err && restrict_stream) {
> + perror("ERROR: Script execution check");
> + return 1;
> + }
> +
> + /* Reads script. */
> + buf_size = fread(buf, 1, buf_size - 1, script);
> + return interpret_buffer(buf, buf_size);
> +}
> +
^ permalink raw reply
* Re: [PATCH v21 1/6] exec: Add a new AT_EXECVE_CHECK flag to execveat(2)
From: Jeff Xu @ 2024-11-21 18:27 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Theodore Ts'o,
Thibaut Sautereau, Vincent Strubel, Xiaoming Ni, Yin Fengwei,
kernel-hardening, linux-api, linux-fsdevel, linux-integrity,
linux-kernel, linux-security-module, Eric Paris, audit
In-Reply-To: <20241121.uquee7ohRohn@digikod.net>
On Thu, Nov 21, 2024 at 5:40 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> On Wed, Nov 20, 2024 at 08:06:07AM -0800, Jeff Xu wrote:
> > On Wed, Nov 20, 2024 at 1:42 AM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > On Tue, Nov 19, 2024 at 05:17:00PM -0800, Jeff Xu wrote:
> > > > On Tue, Nov 12, 2024 at 11:22 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > > >
> > > > > Add a new AT_EXECVE_CHECK flag to execveat(2) to check if a file would
> > > > > be allowed for execution. The main use case is for script interpreters
> > > > > and dynamic linkers to check execution permission according to the
> > > > > kernel's security policy. Another use case is to add context to access
> > > > > logs e.g., which script (instead of interpreter) accessed a file. As
> > > > > any executable code, scripts could also use this check [1].
> > > > >
> > > > > This is different from faccessat(2) + X_OK which only checks a subset of
> > > > > access rights (i.e. inode permission and mount options for regular
> > > > > files), but not the full context (e.g. all LSM access checks). The main
> > > > > use case for access(2) is for SUID processes to (partially) check access
> > > > > on behalf of their caller. The main use case for execveat(2) +
> > > > > AT_EXECVE_CHECK is to check if a script execution would be allowed,
> > > > > according to all the different restrictions in place. Because the use
> > > > > of AT_EXECVE_CHECK follows the exact kernel semantic as for a real
> > > > > execution, user space gets the same error codes.
> > > > >
> > > > > An interesting point of using execveat(2) instead of openat2(2) is that
> > > > > it decouples the check from the enforcement. Indeed, the security check
> > > > > can be logged (e.g. with audit) without blocking an execution
> > > > > environment not yet ready to enforce a strict security policy.
> > > > >
> > > > > LSMs can control or log execution requests with
> > > > > security_bprm_creds_for_exec(). However, to enforce a consistent and
> > > > > complete access control (e.g. on binary's dependencies) LSMs should
> > > > > restrict file executability, or mesure executed files, with
> > > > > security_file_open() by checking file->f_flags & __FMODE_EXEC.
> > > > >
> > > > > Because AT_EXECVE_CHECK is dedicated to user space interpreters, it
> > > > > doesn't make sense for the kernel to parse the checked files, look for
> > > > > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > > > > if the format is unknown. Because of that, security_bprm_check() is
> > > > > never called when AT_EXECVE_CHECK is used.
> > > > >
> > > > > It should be noted that script interpreters cannot directly use
> > > > > execveat(2) (without this new AT_EXECVE_CHECK flag) because this could
> > > > > lead to unexpected behaviors e.g., `python script.sh` could lead to Bash
> > > > > being executed to interpret the script. Unlike the kernel, script
> > > > > interpreters may just interpret the shebang as a simple comment, which
> > > > > should not change for backward compatibility reasons.
> > > > >
> > > > > Because scripts or libraries files might not currently have the
> > > > > executable permission set, or because we might want specific users to be
> > > > > allowed to run arbitrary scripts, the following patch provides a dynamic
> > > > > configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> > > > > SECBIT_EXEC_DENY_INTERACTIVE securebits.
> > > > >
> > > > > This is a redesign of the CLIP OS 4's O_MAYEXEC:
> > > > > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > > > > This patch has been used for more than a decade with customized script
> > > > > interpreters. Some examples can be found here:
> > > > > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> > > > >
> > > > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > > Cc: Kees Cook <keescook@chromium.org>
> > > > > Cc: Paul Moore <paul@paul-moore.com>
> > > > > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> > > > > Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> > > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > > Link: https://lore.kernel.org/r/20241112191858.162021-2-mic@digikod.net
> > > > > ---
> > > > >
> > > > > Changes since v20:
> > > > > * Rename AT_CHECK to AT_EXECVE_CHECK, requested by Amir Goldstein and
> > > > > Serge Hallyn.
> > > > > * Move the UAPI documentation to a dedicated RST file.
> > > > > * Add Reviewed-by: Serge Hallyn
> > > > >
> > > > > Changes since v19:
> > > > > * Remove mention of "role transition" as suggested by Andy.
> > > > > * Highlight the difference between security_bprm_creds_for_exec() and
> > > > > the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> > > > > discussed with Jeff.
> > > > > * Improve documentation both in UAPI comments and kernel comments
> > > > > (requested by Kees).
> > > > >
> > > > > New design since v18:
> > > > > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > > > > ---
> > > > > Documentation/userspace-api/check_exec.rst | 34 ++++++++++++++++++++++
> > > > > Documentation/userspace-api/index.rst | 1 +
> > > > > fs/exec.c | 20 +++++++++++--
> > > > > include/linux/binfmts.h | 7 ++++-
> > > > > include/uapi/linux/fcntl.h | 4 +++
> > > > > kernel/audit.h | 1 +
> > > > > kernel/auditsc.c | 1 +
> > > > > security/security.c | 10 +++++++
> > > > > 8 files changed, 75 insertions(+), 3 deletions(-)
> > > > > create mode 100644 Documentation/userspace-api/check_exec.rst
> > > > >
> > > > > diff --git a/Documentation/userspace-api/check_exec.rst b/Documentation/userspace-api/check_exec.rst
> > > > > new file mode 100644
> > > > > index 000000000000..ad1aeaa5f6c0
> > > > > --- /dev/null
> > > > > +++ b/Documentation/userspace-api/check_exec.rst
> > > > > @@ -0,0 +1,34 @@
> > > > > +===================
> > > > > +Executability check
> > > > > +===================
> > > > > +
> > > > > +AT_EXECVE_CHECK
> > > > > +===============
> > > > > +
> > > > > +Passing the ``AT_EXECVE_CHECK`` flag to :manpage:`execveat(2)` only performs a
> > > > > +check on a regular file and returns 0 if execution of this file would be
> > > > > +allowed, ignoring the file format and then the related interpreter dependencies
> > > > > +(e.g. ELF libraries, script's shebang).
> > > > > +
> > > > > +Programs should always perform this check to apply kernel-level checks against
> > > > > +files that are not directly executed by the kernel but passed to a user space
> > > > > +interpreter instead. All files that contain executable code, from the point of
> > > > > +view of the interpreter, should be checked. However the result of this check
> > > > > +should only be enforced according to ``SECBIT_EXEC_RESTRICT_FILE`` or
> > > > > +``SECBIT_EXEC_DENY_INTERACTIVE.``.
> > > > Regarding "should only"
> > > > Userspace (e.g. libc) could decide to enforce even when
> > > > SECBIT_EXEC_RESTRICT_FILE=0), i.e. if it determines not-enforcing
> > > > doesn't make sense.
> > >
> > > User space is always in control, but I don't think it would be wise to
> > > not follow the configuration securebits (in a generic system) because
> > > this could result to unattended behaviors (I don't have a specific one
> > > in mind but...). That being said, configuration and checks are
> > > standalones and specific/tailored systems are free to do the checks they
> > > want.
> > >
> > In the case of dynamic linker, we can always enforce honoring the
> > execveat(AT_EXECVE_CHECK) result, right ? I can't think of a case not
> > to, the dynamic linker doesn't need to check the
> > SECBIT_EXEC_RESTRICT_FILE bit.
>
> If the library file is not allowed to be executed by *all* access
> control systems (not just mount and file permission, but all LSMs), then
> the AT_EXECVE_CHECK will fail, which is OK as long as it is not a hard
> requirement.
Yes. specifically for the library loading case, I can't think of a
case where we need to by-pass LSMs. (letting user space to by-pass
LSM check seems questionable in concept, and should only be used when
there aren't other solutions). In the context of SELINUX enforcing
mode, we will want to enforce it. In the context of process level LSM
such as landlock, the process can already decide for itself by
selecting the policy for its own domain, it is unnecessary to use
another opt-out solution.
There is one case where I see a difference:
ld.so a.out (when a.out is on non-exec mount)
If the dynamic linker doesn't read SECBIT_EXEC_RESTRICT_FILE setting,
above will always fail. But that is more of a bugfix.
>Relying on the securebits to know if this is a hard
> requirement or not enables system administrator and distros to control
> this potential behavior change.
>
I think, for the dynamic linker, it can be a hard requirement.
For scripts, the cases are more complicated and we can't just enforce
it, therefore have to rely on security bits to give a pre-process
level control.
> >
> > script interpreters need to check this though, because the apps might
> > need to adjust/test the scripts they are calling, so
> > SECBIT_EXEC_RESTRICT_FILE can be used to opt-out the enforcement.
> >
> > > > When SECBIT_EXEC_RESTRICT_FILE=1, userspace is bound to enforce.
> > > >
> > > > > +
> > > > > +The main purpose of this flag is to improve the security and consistency of an
> > > > > +execution environment to ensure that direct file execution (e.g.
> > > > > +``./script.sh``) and indirect file execution (e.g. ``sh script.sh``) lead to
> > > > > +the same result. For instance, this can be used to check if a file is
> > > > > +trustworthy according to the caller's environment.
> > > > > +
> > > > > +In a secure environment, libraries and any executable dependencies should also
> > > > > +be checked. For instance, dynamic linking should make sure that all libraries
> > > > > +are allowed for execution to avoid trivial bypass (e.g. using ``LD_PRELOAD``).
> > > > > +For such secure execution environment to make sense, only trusted code should
> > > > > +be executable, which also requires integrity guarantees.
> > > > > +
> > > > > +To avoid race conditions leading to time-of-check to time-of-use issues,
> > > > > +``AT_EXECVE_CHECK`` should be used with ``AT_EMPTY_PATH`` to check against a
> > > > > +file descriptor instead of a path.
> > > > > diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
> > > > > index 274cc7546efc..6272bcf11296 100644
> > > > > --- a/Documentation/userspace-api/index.rst
> > > > > +++ b/Documentation/userspace-api/index.rst
> > > > > @@ -35,6 +35,7 @@ Security-related interfaces
> > > > > mfd_noexec
> > > > > spec_ctrl
> > > > > tee
> > > > > + check_exec
> > > > >
> > > > > Devices and I/O
> > > > > ===============
> > > > > diff --git a/fs/exec.c b/fs/exec.c
> > > > > index 6c53920795c2..bb83b6a39530 100644
> > > > > --- a/fs/exec.c
> > > > > +++ b/fs/exec.c
> > > > > @@ -891,7 +891,8 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > > > > .lookup_flags = LOOKUP_FOLLOW,
> > > > > };
> > > > >
> > > > > - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > > > > + if ((flags &
> > > > > + ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_EXECVE_CHECK)) != 0)
> > > > > return ERR_PTR(-EINVAL);
> > > > > if (flags & AT_SYMLINK_NOFOLLOW)
> > > > > open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> > > > > @@ -1545,6 +1546,21 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
> > > > > }
> > > > > bprm->interp = bprm->filename;
> > > > >
> > > > > + /*
> > > > > + * At this point, security_file_open() has already been called (with
> > > > > + * __FMODE_EXEC) and access control checks for AT_EXECVE_CHECK will
> > > > > + * stop just after the security_bprm_creds_for_exec() call in
> > > > > + * bprm_execve(). Indeed, the kernel should not try to parse the
> > > > > + * content of the file with exec_binprm() nor change the calling
> > > > > + * thread, which means that the following security functions will be
> > > > > + * not called:
> > > > > + * - security_bprm_check()
> > > > > + * - security_bprm_creds_from_file()
> > > > > + * - security_bprm_committing_creds()
> > > > > + * - security_bprm_committed_creds()
> > > > > + */
> > > > > + bprm->is_check = !!(flags & AT_EXECVE_CHECK);
> > > > > +
> > > > > retval = bprm_mm_init(bprm);
> > > > > if (!retval)
> > > > > return bprm;
> > > > > @@ -1839,7 +1855,7 @@ static int bprm_execve(struct linux_binprm *bprm)
> > > > >
> > > > > /* Set the unchanging part of bprm->cred */
> > > > > retval = security_bprm_creds_for_exec(bprm);
> > > > > - if (retval)
> > > > > + if (retval || bprm->is_check)
> > > > > goto out;
> > > > >
> > > > > retval = exec_binprm(bprm);
> > > > > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> > > > > index e6c00e860951..8ff0eb3644a1 100644
> > > > > --- a/include/linux/binfmts.h
> > > > > +++ b/include/linux/binfmts.h
> > > > > @@ -42,7 +42,12 @@ struct linux_binprm {
> > > > > * Set when errors can no longer be returned to the
> > > > > * original userspace.
> > > > > */
> > > > > - point_of_no_return:1;
> > > > > + point_of_no_return:1,
> > > > > + /*
> > > > > + * Set by user space to check executability according to the
> > > > > + * caller's environment.
> > > > > + */
> > > > > + is_check:1;
> > > > > struct file *executable; /* Executable to pass to the interpreter */
> > > > > struct file *interpreter;
> > > > > struct file *file;
> > > > > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > > > > index 87e2dec79fea..2e87f2e3a79f 100644
> > > > > --- a/include/uapi/linux/fcntl.h
> > > > > +++ b/include/uapi/linux/fcntl.h
> > > > > @@ -154,6 +154,10 @@
> > > > > usable with open_by_handle_at(2). */
> > > > > #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
> > > > >
> > > > > +/* Flags for execveat2(2). */
> > > > > +#define AT_EXECVE_CHECK 0x10000 /* Only perform a check if execution
> > > > > + would be allowed. */
> > > > > +
> > > > > #if defined(__KERNEL__)
> > > > > #define AT_GETATTR_NOSEC 0x80000000
> > > > > #endif
> > > > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > > > index a60d2840559e..8ebdabd2ab81 100644
> > > > > --- a/kernel/audit.h
> > > > > +++ b/kernel/audit.h
> > > > > @@ -197,6 +197,7 @@ struct audit_context {
> > > > > struct open_how openat2;
> > > > > struct {
> > > > > int argc;
> > > > > + bool is_check;
> > > > > } execve;
> > > > > struct {
> > > > > char *name;
> > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > index cd57053b4a69..8d9ba5600cf2 100644
> > > > > --- a/kernel/auditsc.c
> > > > > +++ b/kernel/auditsc.c
> > > > > @@ -2662,6 +2662,7 @@ void __audit_bprm(struct linux_binprm *bprm)
> > > > >
> > > > > context->type = AUDIT_EXECVE;
> > > > > context->execve.argc = bprm->argc;
> > > > > + context->execve.is_check = bprm->is_check;
> > > > Where is execve.is_check used ?
> > >
> > > It is used in bprm_execve(), exposed to the audit framework, and
> > > potentially used by LSMs.
> > >
> > bprm_execve() uses bprm->is_check, not the context->execve.is_check.
>
> Correct, this is only for audit but not used yet.
>
> Paul, Eric, do you want me to remove this field, leave it, or extend
> this patch like this?
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 8d9ba5600cf2..12cf89fa224a 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1290,6 +1290,8 @@ static void audit_log_execve_info(struct audit_context *context,
> }
> } while (arg < context->execve.argc);
>
> + audit_log_format(*ab, " check=%d", context->execve.is_check);
> +
> /* NOTE: the caller handles the final audit_log_end() call */
>
> out:
>
> >
> >
> > > >
> > > >
> > > > > }
> > > > >
> > > > >
> > > > > diff --git a/security/security.c b/security/security.c
> > > > > index c5981e558bc2..456361ec249d 100644
> > > > > --- a/security/security.c
> > > > > +++ b/security/security.c
> > > > > @@ -1249,6 +1249,12 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> > > > > * to 1 if AT_SECURE should be set to request libc enable secure mode. @bprm
> > > > > * contains the linux_binprm structure.
> > > > > *
> > > > > + * If execveat(2) is called with the AT_EXECVE_CHECK flag, bprm->is_check is
> > > > > + * set. The result must be the same as without this flag even if the execution
> > > > > + * will never really happen and @bprm will always be dropped.
> > > > > + *
> > > > > + * This hook must not change current->cred, only @bprm->cred.
> > > > > + *
> > > > > * Return: Returns 0 if the hook is successful and permission is granted.
> > > > > */
> > > > > int security_bprm_creds_for_exec(struct linux_binprm *bprm)
> > > > > @@ -3100,6 +3106,10 @@ int security_file_receive(struct file *file)
> > > > > * Save open-time permission checking state for later use upon file_permission,
> > > > > * and recheck access if anything has changed since inode_permission.
> > > > > *
> > > > > + * We can check if a file is opened for execution (e.g. execve(2) call), either
> > > > > + * directly or indirectly (e.g. ELF's ld.so) by checking file->f_flags &
> > > > > + * __FMODE_EXEC .
> > > > > + *
> > > > > * Return: Returns 0 if permission is granted.
> > > > > */
> > > > > int security_file_open(struct file *file)
> > > > > --
> > > > > 2.47.0
> > > > >
> > > > >
> >
^ permalink raw reply
* Re: [PATCH v21 1/6] exec: Add a new AT_EXECVE_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-11-21 13:39 UTC (permalink / raw)
To: Jeff Xu
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Theodore Ts'o,
Thibaut Sautereau, Vincent Strubel, Xiaoming Ni, Yin Fengwei,
kernel-hardening, linux-api, linux-fsdevel, linux-integrity,
linux-kernel, linux-security-module, Eric Paris, audit
In-Reply-To: <CABi2SkUx=7zummB4JCqEfb37p6MORR88y7S0E_YxJND_8dGaKA@mail.gmail.com>
On Wed, Nov 20, 2024 at 08:06:07AM -0800, Jeff Xu wrote:
> On Wed, Nov 20, 2024 at 1:42 AM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > On Tue, Nov 19, 2024 at 05:17:00PM -0800, Jeff Xu wrote:
> > > On Tue, Nov 12, 2024 at 11:22 AM Mickaël Salaün <mic@digikod.net> wrote:
> > > >
> > > > Add a new AT_EXECVE_CHECK flag to execveat(2) to check if a file would
> > > > be allowed for execution. The main use case is for script interpreters
> > > > and dynamic linkers to check execution permission according to the
> > > > kernel's security policy. Another use case is to add context to access
> > > > logs e.g., which script (instead of interpreter) accessed a file. As
> > > > any executable code, scripts could also use this check [1].
> > > >
> > > > This is different from faccessat(2) + X_OK which only checks a subset of
> > > > access rights (i.e. inode permission and mount options for regular
> > > > files), but not the full context (e.g. all LSM access checks). The main
> > > > use case for access(2) is for SUID processes to (partially) check access
> > > > on behalf of their caller. The main use case for execveat(2) +
> > > > AT_EXECVE_CHECK is to check if a script execution would be allowed,
> > > > according to all the different restrictions in place. Because the use
> > > > of AT_EXECVE_CHECK follows the exact kernel semantic as for a real
> > > > execution, user space gets the same error codes.
> > > >
> > > > An interesting point of using execveat(2) instead of openat2(2) is that
> > > > it decouples the check from the enforcement. Indeed, the security check
> > > > can be logged (e.g. with audit) without blocking an execution
> > > > environment not yet ready to enforce a strict security policy.
> > > >
> > > > LSMs can control or log execution requests with
> > > > security_bprm_creds_for_exec(). However, to enforce a consistent and
> > > > complete access control (e.g. on binary's dependencies) LSMs should
> > > > restrict file executability, or mesure executed files, with
> > > > security_file_open() by checking file->f_flags & __FMODE_EXEC.
> > > >
> > > > Because AT_EXECVE_CHECK is dedicated to user space interpreters, it
> > > > doesn't make sense for the kernel to parse the checked files, look for
> > > > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > > > if the format is unknown. Because of that, security_bprm_check() is
> > > > never called when AT_EXECVE_CHECK is used.
> > > >
> > > > It should be noted that script interpreters cannot directly use
> > > > execveat(2) (without this new AT_EXECVE_CHECK flag) because this could
> > > > lead to unexpected behaviors e.g., `python script.sh` could lead to Bash
> > > > being executed to interpret the script. Unlike the kernel, script
> > > > interpreters may just interpret the shebang as a simple comment, which
> > > > should not change for backward compatibility reasons.
> > > >
> > > > Because scripts or libraries files might not currently have the
> > > > executable permission set, or because we might want specific users to be
> > > > allowed to run arbitrary scripts, the following patch provides a dynamic
> > > > configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> > > > SECBIT_EXEC_DENY_INTERACTIVE securebits.
> > > >
> > > > This is a redesign of the CLIP OS 4's O_MAYEXEC:
> > > > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > > > This patch has been used for more than a decade with customized script
> > > > interpreters. Some examples can be found here:
> > > > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> > > >
> > > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > > Cc: Christian Brauner <brauner@kernel.org>
> > > > Cc: Kees Cook <keescook@chromium.org>
> > > > Cc: Paul Moore <paul@paul-moore.com>
> > > > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> > > > Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> > > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > > Link: https://lore.kernel.org/r/20241112191858.162021-2-mic@digikod.net
> > > > ---
> > > >
> > > > Changes since v20:
> > > > * Rename AT_CHECK to AT_EXECVE_CHECK, requested by Amir Goldstein and
> > > > Serge Hallyn.
> > > > * Move the UAPI documentation to a dedicated RST file.
> > > > * Add Reviewed-by: Serge Hallyn
> > > >
> > > > Changes since v19:
> > > > * Remove mention of "role transition" as suggested by Andy.
> > > > * Highlight the difference between security_bprm_creds_for_exec() and
> > > > the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> > > > discussed with Jeff.
> > > > * Improve documentation both in UAPI comments and kernel comments
> > > > (requested by Kees).
> > > >
> > > > New design since v18:
> > > > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > > > ---
> > > > Documentation/userspace-api/check_exec.rst | 34 ++++++++++++++++++++++
> > > > Documentation/userspace-api/index.rst | 1 +
> > > > fs/exec.c | 20 +++++++++++--
> > > > include/linux/binfmts.h | 7 ++++-
> > > > include/uapi/linux/fcntl.h | 4 +++
> > > > kernel/audit.h | 1 +
> > > > kernel/auditsc.c | 1 +
> > > > security/security.c | 10 +++++++
> > > > 8 files changed, 75 insertions(+), 3 deletions(-)
> > > > create mode 100644 Documentation/userspace-api/check_exec.rst
> > > >
> > > > diff --git a/Documentation/userspace-api/check_exec.rst b/Documentation/userspace-api/check_exec.rst
> > > > new file mode 100644
> > > > index 000000000000..ad1aeaa5f6c0
> > > > --- /dev/null
> > > > +++ b/Documentation/userspace-api/check_exec.rst
> > > > @@ -0,0 +1,34 @@
> > > > +===================
> > > > +Executability check
> > > > +===================
> > > > +
> > > > +AT_EXECVE_CHECK
> > > > +===============
> > > > +
> > > > +Passing the ``AT_EXECVE_CHECK`` flag to :manpage:`execveat(2)` only performs a
> > > > +check on a regular file and returns 0 if execution of this file would be
> > > > +allowed, ignoring the file format and then the related interpreter dependencies
> > > > +(e.g. ELF libraries, script's shebang).
> > > > +
> > > > +Programs should always perform this check to apply kernel-level checks against
> > > > +files that are not directly executed by the kernel but passed to a user space
> > > > +interpreter instead. All files that contain executable code, from the point of
> > > > +view of the interpreter, should be checked. However the result of this check
> > > > +should only be enforced according to ``SECBIT_EXEC_RESTRICT_FILE`` or
> > > > +``SECBIT_EXEC_DENY_INTERACTIVE.``.
> > > Regarding "should only"
> > > Userspace (e.g. libc) could decide to enforce even when
> > > SECBIT_EXEC_RESTRICT_FILE=0), i.e. if it determines not-enforcing
> > > doesn't make sense.
> >
> > User space is always in control, but I don't think it would be wise to
> > not follow the configuration securebits (in a generic system) because
> > this could result to unattended behaviors (I don't have a specific one
> > in mind but...). That being said, configuration and checks are
> > standalones and specific/tailored systems are free to do the checks they
> > want.
> >
> In the case of dynamic linker, we can always enforce honoring the
> execveat(AT_EXECVE_CHECK) result, right ? I can't think of a case not
> to, the dynamic linker doesn't need to check the
> SECBIT_EXEC_RESTRICT_FILE bit.
If the library file is not allowed to be executed by *all* access
control systems (not just mount and file permission, but all LSMs), then
the AT_EXECVE_CHECK will fail, which is OK as long as it is not a hard
requirement. Relying on the securebits to know if this is a hard
requirement or not enables system administrator and distros to control
this potential behavior change.
>
> script interpreters need to check this though, because the apps might
> need to adjust/test the scripts they are calling, so
> SECBIT_EXEC_RESTRICT_FILE can be used to opt-out the enforcement.
>
> > > When SECBIT_EXEC_RESTRICT_FILE=1, userspace is bound to enforce.
> > >
> > > > +
> > > > +The main purpose of this flag is to improve the security and consistency of an
> > > > +execution environment to ensure that direct file execution (e.g.
> > > > +``./script.sh``) and indirect file execution (e.g. ``sh script.sh``) lead to
> > > > +the same result. For instance, this can be used to check if a file is
> > > > +trustworthy according to the caller's environment.
> > > > +
> > > > +In a secure environment, libraries and any executable dependencies should also
> > > > +be checked. For instance, dynamic linking should make sure that all libraries
> > > > +are allowed for execution to avoid trivial bypass (e.g. using ``LD_PRELOAD``).
> > > > +For such secure execution environment to make sense, only trusted code should
> > > > +be executable, which also requires integrity guarantees.
> > > > +
> > > > +To avoid race conditions leading to time-of-check to time-of-use issues,
> > > > +``AT_EXECVE_CHECK`` should be used with ``AT_EMPTY_PATH`` to check against a
> > > > +file descriptor instead of a path.
> > > > diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
> > > > index 274cc7546efc..6272bcf11296 100644
> > > > --- a/Documentation/userspace-api/index.rst
> > > > +++ b/Documentation/userspace-api/index.rst
> > > > @@ -35,6 +35,7 @@ Security-related interfaces
> > > > mfd_noexec
> > > > spec_ctrl
> > > > tee
> > > > + check_exec
> > > >
> > > > Devices and I/O
> > > > ===============
> > > > diff --git a/fs/exec.c b/fs/exec.c
> > > > index 6c53920795c2..bb83b6a39530 100644
> > > > --- a/fs/exec.c
> > > > +++ b/fs/exec.c
> > > > @@ -891,7 +891,8 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > > > .lookup_flags = LOOKUP_FOLLOW,
> > > > };
> > > >
> > > > - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > > > + if ((flags &
> > > > + ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_EXECVE_CHECK)) != 0)
> > > > return ERR_PTR(-EINVAL);
> > > > if (flags & AT_SYMLINK_NOFOLLOW)
> > > > open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> > > > @@ -1545,6 +1546,21 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
> > > > }
> > > > bprm->interp = bprm->filename;
> > > >
> > > > + /*
> > > > + * At this point, security_file_open() has already been called (with
> > > > + * __FMODE_EXEC) and access control checks for AT_EXECVE_CHECK will
> > > > + * stop just after the security_bprm_creds_for_exec() call in
> > > > + * bprm_execve(). Indeed, the kernel should not try to parse the
> > > > + * content of the file with exec_binprm() nor change the calling
> > > > + * thread, which means that the following security functions will be
> > > > + * not called:
> > > > + * - security_bprm_check()
> > > > + * - security_bprm_creds_from_file()
> > > > + * - security_bprm_committing_creds()
> > > > + * - security_bprm_committed_creds()
> > > > + */
> > > > + bprm->is_check = !!(flags & AT_EXECVE_CHECK);
> > > > +
> > > > retval = bprm_mm_init(bprm);
> > > > if (!retval)
> > > > return bprm;
> > > > @@ -1839,7 +1855,7 @@ static int bprm_execve(struct linux_binprm *bprm)
> > > >
> > > > /* Set the unchanging part of bprm->cred */
> > > > retval = security_bprm_creds_for_exec(bprm);
> > > > - if (retval)
> > > > + if (retval || bprm->is_check)
> > > > goto out;
> > > >
> > > > retval = exec_binprm(bprm);
> > > > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> > > > index e6c00e860951..8ff0eb3644a1 100644
> > > > --- a/include/linux/binfmts.h
> > > > +++ b/include/linux/binfmts.h
> > > > @@ -42,7 +42,12 @@ struct linux_binprm {
> > > > * Set when errors can no longer be returned to the
> > > > * original userspace.
> > > > */
> > > > - point_of_no_return:1;
> > > > + point_of_no_return:1,
> > > > + /*
> > > > + * Set by user space to check executability according to the
> > > > + * caller's environment.
> > > > + */
> > > > + is_check:1;
> > > > struct file *executable; /* Executable to pass to the interpreter */
> > > > struct file *interpreter;
> > > > struct file *file;
> > > > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > > > index 87e2dec79fea..2e87f2e3a79f 100644
> > > > --- a/include/uapi/linux/fcntl.h
> > > > +++ b/include/uapi/linux/fcntl.h
> > > > @@ -154,6 +154,10 @@
> > > > usable with open_by_handle_at(2). */
> > > > #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
> > > >
> > > > +/* Flags for execveat2(2). */
> > > > +#define AT_EXECVE_CHECK 0x10000 /* Only perform a check if execution
> > > > + would be allowed. */
> > > > +
> > > > #if defined(__KERNEL__)
> > > > #define AT_GETATTR_NOSEC 0x80000000
> > > > #endif
> > > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > > index a60d2840559e..8ebdabd2ab81 100644
> > > > --- a/kernel/audit.h
> > > > +++ b/kernel/audit.h
> > > > @@ -197,6 +197,7 @@ struct audit_context {
> > > > struct open_how openat2;
> > > > struct {
> > > > int argc;
> > > > + bool is_check;
> > > > } execve;
> > > > struct {
> > > > char *name;
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index cd57053b4a69..8d9ba5600cf2 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -2662,6 +2662,7 @@ void __audit_bprm(struct linux_binprm *bprm)
> > > >
> > > > context->type = AUDIT_EXECVE;
> > > > context->execve.argc = bprm->argc;
> > > > + context->execve.is_check = bprm->is_check;
> > > Where is execve.is_check used ?
> >
> > It is used in bprm_execve(), exposed to the audit framework, and
> > potentially used by LSMs.
> >
> bprm_execve() uses bprm->is_check, not the context->execve.is_check.
Correct, this is only for audit but not used yet.
Paul, Eric, do you want me to remove this field, leave it, or extend
this patch like this?
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 8d9ba5600cf2..12cf89fa224a 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1290,6 +1290,8 @@ static void audit_log_execve_info(struct audit_context *context,
}
} while (arg < context->execve.argc);
+ audit_log_format(*ab, " check=%d", context->execve.is_check);
+
/* NOTE: the caller handles the final audit_log_end() call */
out:
>
>
> > >
> > >
> > > > }
> > > >
> > > >
> > > > diff --git a/security/security.c b/security/security.c
> > > > index c5981e558bc2..456361ec249d 100644
> > > > --- a/security/security.c
> > > > +++ b/security/security.c
> > > > @@ -1249,6 +1249,12 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> > > > * to 1 if AT_SECURE should be set to request libc enable secure mode. @bprm
> > > > * contains the linux_binprm structure.
> > > > *
> > > > + * If execveat(2) is called with the AT_EXECVE_CHECK flag, bprm->is_check is
> > > > + * set. The result must be the same as without this flag even if the execution
> > > > + * will never really happen and @bprm will always be dropped.
> > > > + *
> > > > + * This hook must not change current->cred, only @bprm->cred.
> > > > + *
> > > > * Return: Returns 0 if the hook is successful and permission is granted.
> > > > */
> > > > int security_bprm_creds_for_exec(struct linux_binprm *bprm)
> > > > @@ -3100,6 +3106,10 @@ int security_file_receive(struct file *file)
> > > > * Save open-time permission checking state for later use upon file_permission,
> > > > * and recheck access if anything has changed since inode_permission.
> > > > *
> > > > + * We can check if a file is opened for execution (e.g. execve(2) call), either
> > > > + * directly or indirectly (e.g. ELF's ld.so) by checking file->f_flags &
> > > > + * __FMODE_EXEC .
> > > > + *
> > > > * Return: Returns 0 if permission is granted.
> > > > */
> > > > int security_file_open(struct file *file)
> > > > --
> > > > 2.47.0
> > > >
> > > >
>
^ permalink raw reply related
* Re: [PATCH v21 0/6] Script execution control (was O_MAYEXEC)
From: Kees Cook @ 2024-11-21 4:58 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Theodore Ts'o,
Thibaut Sautereau, Vincent Strubel, Xiaoming Ni, Yin Fengwei,
kernel-hardening, linux-api, linux-fsdevel, linux-integrity,
linux-kernel, linux-security-module
In-Reply-To: <20241112191858.162021-1-mic@digikod.net>
On Tue, Nov 12, 2024 at 08:18:52PM +0100, Mickaël Salaün wrote:
> Kees, would you like to take this series in your tree?
Yeah, let's give it a shot for -next after the merge window is closed,
assuming review is clean.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v21 1/6] exec: Add a new AT_EXECVE_CHECK flag to execveat(2)
From: Jeff Xu @ 2024-11-20 16:06 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Theodore Ts'o,
Thibaut Sautereau, Vincent Strubel, Xiaoming Ni, Yin Fengwei,
kernel-hardening, linux-api, linux-fsdevel, linux-integrity,
linux-kernel, linux-security-module
In-Reply-To: <20241120.Uy8ahtai5oku@digikod.net>
On Wed, Nov 20, 2024 at 1:42 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> On Tue, Nov 19, 2024 at 05:17:00PM -0800, Jeff Xu wrote:
> > On Tue, Nov 12, 2024 at 11:22 AM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > Add a new AT_EXECVE_CHECK flag to execveat(2) to check if a file would
> > > be allowed for execution. The main use case is for script interpreters
> > > and dynamic linkers to check execution permission according to the
> > > kernel's security policy. Another use case is to add context to access
> > > logs e.g., which script (instead of interpreter) accessed a file. As
> > > any executable code, scripts could also use this check [1].
> > >
> > > This is different from faccessat(2) + X_OK which only checks a subset of
> > > access rights (i.e. inode permission and mount options for regular
> > > files), but not the full context (e.g. all LSM access checks). The main
> > > use case for access(2) is for SUID processes to (partially) check access
> > > on behalf of their caller. The main use case for execveat(2) +
> > > AT_EXECVE_CHECK is to check if a script execution would be allowed,
> > > according to all the different restrictions in place. Because the use
> > > of AT_EXECVE_CHECK follows the exact kernel semantic as for a real
> > > execution, user space gets the same error codes.
> > >
> > > An interesting point of using execveat(2) instead of openat2(2) is that
> > > it decouples the check from the enforcement. Indeed, the security check
> > > can be logged (e.g. with audit) without blocking an execution
> > > environment not yet ready to enforce a strict security policy.
> > >
> > > LSMs can control or log execution requests with
> > > security_bprm_creds_for_exec(). However, to enforce a consistent and
> > > complete access control (e.g. on binary's dependencies) LSMs should
> > > restrict file executability, or mesure executed files, with
> > > security_file_open() by checking file->f_flags & __FMODE_EXEC.
> > >
> > > Because AT_EXECVE_CHECK is dedicated to user space interpreters, it
> > > doesn't make sense for the kernel to parse the checked files, look for
> > > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > > if the format is unknown. Because of that, security_bprm_check() is
> > > never called when AT_EXECVE_CHECK is used.
> > >
> > > It should be noted that script interpreters cannot directly use
> > > execveat(2) (without this new AT_EXECVE_CHECK flag) because this could
> > > lead to unexpected behaviors e.g., `python script.sh` could lead to Bash
> > > being executed to interpret the script. Unlike the kernel, script
> > > interpreters may just interpret the shebang as a simple comment, which
> > > should not change for backward compatibility reasons.
> > >
> > > Because scripts or libraries files might not currently have the
> > > executable permission set, or because we might want specific users to be
> > > allowed to run arbitrary scripts, the following patch provides a dynamic
> > > configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> > > SECBIT_EXEC_DENY_INTERACTIVE securebits.
> > >
> > > This is a redesign of the CLIP OS 4's O_MAYEXEC:
> > > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > > This patch has been used for more than a decade with customized script
> > > interpreters. Some examples can be found here:
> > > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> > >
> > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Paul Moore <paul@paul-moore.com>
> > > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> > > Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > Link: https://lore.kernel.org/r/20241112191858.162021-2-mic@digikod.net
> > > ---
> > >
> > > Changes since v20:
> > > * Rename AT_CHECK to AT_EXECVE_CHECK, requested by Amir Goldstein and
> > > Serge Hallyn.
> > > * Move the UAPI documentation to a dedicated RST file.
> > > * Add Reviewed-by: Serge Hallyn
> > >
> > > Changes since v19:
> > > * Remove mention of "role transition" as suggested by Andy.
> > > * Highlight the difference between security_bprm_creds_for_exec() and
> > > the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> > > discussed with Jeff.
> > > * Improve documentation both in UAPI comments and kernel comments
> > > (requested by Kees).
> > >
> > > New design since v18:
> > > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > > ---
> > > Documentation/userspace-api/check_exec.rst | 34 ++++++++++++++++++++++
> > > Documentation/userspace-api/index.rst | 1 +
> > > fs/exec.c | 20 +++++++++++--
> > > include/linux/binfmts.h | 7 ++++-
> > > include/uapi/linux/fcntl.h | 4 +++
> > > kernel/audit.h | 1 +
> > > kernel/auditsc.c | 1 +
> > > security/security.c | 10 +++++++
> > > 8 files changed, 75 insertions(+), 3 deletions(-)
> > > create mode 100644 Documentation/userspace-api/check_exec.rst
> > >
> > > diff --git a/Documentation/userspace-api/check_exec.rst b/Documentation/userspace-api/check_exec.rst
> > > new file mode 100644
> > > index 000000000000..ad1aeaa5f6c0
> > > --- /dev/null
> > > +++ b/Documentation/userspace-api/check_exec.rst
> > > @@ -0,0 +1,34 @@
> > > +===================
> > > +Executability check
> > > +===================
> > > +
> > > +AT_EXECVE_CHECK
> > > +===============
> > > +
> > > +Passing the ``AT_EXECVE_CHECK`` flag to :manpage:`execveat(2)` only performs a
> > > +check on a regular file and returns 0 if execution of this file would be
> > > +allowed, ignoring the file format and then the related interpreter dependencies
> > > +(e.g. ELF libraries, script's shebang).
> > > +
> > > +Programs should always perform this check to apply kernel-level checks against
> > > +files that are not directly executed by the kernel but passed to a user space
> > > +interpreter instead. All files that contain executable code, from the point of
> > > +view of the interpreter, should be checked. However the result of this check
> > > +should only be enforced according to ``SECBIT_EXEC_RESTRICT_FILE`` or
> > > +``SECBIT_EXEC_DENY_INTERACTIVE.``.
> > Regarding "should only"
> > Userspace (e.g. libc) could decide to enforce even when
> > SECBIT_EXEC_RESTRICT_FILE=0), i.e. if it determines not-enforcing
> > doesn't make sense.
>
> User space is always in control, but I don't think it would be wise to
> not follow the configuration securebits (in a generic system) because
> this could result to unattended behaviors (I don't have a specific one
> in mind but...). That being said, configuration and checks are
> standalones and specific/tailored systems are free to do the checks they
> want.
>
In the case of dynamic linker, we can always enforce honoring the
execveat(AT_EXECVE_CHECK) result, right ? I can't think of a case not
to, the dynamic linker doesn't need to check the
SECBIT_EXEC_RESTRICT_FILE bit.
script interpreters need to check this though, because the apps might
need to adjust/test the scripts they are calling, so
SECBIT_EXEC_RESTRICT_FILE can be used to opt-out the enforcement.
> > When SECBIT_EXEC_RESTRICT_FILE=1, userspace is bound to enforce.
> >
> > > +
> > > +The main purpose of this flag is to improve the security and consistency of an
> > > +execution environment to ensure that direct file execution (e.g.
> > > +``./script.sh``) and indirect file execution (e.g. ``sh script.sh``) lead to
> > > +the same result. For instance, this can be used to check if a file is
> > > +trustworthy according to the caller's environment.
> > > +
> > > +In a secure environment, libraries and any executable dependencies should also
> > > +be checked. For instance, dynamic linking should make sure that all libraries
> > > +are allowed for execution to avoid trivial bypass (e.g. using ``LD_PRELOAD``).
> > > +For such secure execution environment to make sense, only trusted code should
> > > +be executable, which also requires integrity guarantees.
> > > +
> > > +To avoid race conditions leading to time-of-check to time-of-use issues,
> > > +``AT_EXECVE_CHECK`` should be used with ``AT_EMPTY_PATH`` to check against a
> > > +file descriptor instead of a path.
> > > diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
> > > index 274cc7546efc..6272bcf11296 100644
> > > --- a/Documentation/userspace-api/index.rst
> > > +++ b/Documentation/userspace-api/index.rst
> > > @@ -35,6 +35,7 @@ Security-related interfaces
> > > mfd_noexec
> > > spec_ctrl
> > > tee
> > > + check_exec
> > >
> > > Devices and I/O
> > > ===============
> > > diff --git a/fs/exec.c b/fs/exec.c
> > > index 6c53920795c2..bb83b6a39530 100644
> > > --- a/fs/exec.c
> > > +++ b/fs/exec.c
> > > @@ -891,7 +891,8 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > > .lookup_flags = LOOKUP_FOLLOW,
> > > };
> > >
> > > - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > > + if ((flags &
> > > + ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_EXECVE_CHECK)) != 0)
> > > return ERR_PTR(-EINVAL);
> > > if (flags & AT_SYMLINK_NOFOLLOW)
> > > open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> > > @@ -1545,6 +1546,21 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
> > > }
> > > bprm->interp = bprm->filename;
> > >
> > > + /*
> > > + * At this point, security_file_open() has already been called (with
> > > + * __FMODE_EXEC) and access control checks for AT_EXECVE_CHECK will
> > > + * stop just after the security_bprm_creds_for_exec() call in
> > > + * bprm_execve(). Indeed, the kernel should not try to parse the
> > > + * content of the file with exec_binprm() nor change the calling
> > > + * thread, which means that the following security functions will be
> > > + * not called:
> > > + * - security_bprm_check()
> > > + * - security_bprm_creds_from_file()
> > > + * - security_bprm_committing_creds()
> > > + * - security_bprm_committed_creds()
> > > + */
> > > + bprm->is_check = !!(flags & AT_EXECVE_CHECK);
> > > +
> > > retval = bprm_mm_init(bprm);
> > > if (!retval)
> > > return bprm;
> > > @@ -1839,7 +1855,7 @@ static int bprm_execve(struct linux_binprm *bprm)
> > >
> > > /* Set the unchanging part of bprm->cred */
> > > retval = security_bprm_creds_for_exec(bprm);
> > > - if (retval)
> > > + if (retval || bprm->is_check)
> > > goto out;
> > >
> > > retval = exec_binprm(bprm);
> > > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> > > index e6c00e860951..8ff0eb3644a1 100644
> > > --- a/include/linux/binfmts.h
> > > +++ b/include/linux/binfmts.h
> > > @@ -42,7 +42,12 @@ struct linux_binprm {
> > > * Set when errors can no longer be returned to the
> > > * original userspace.
> > > */
> > > - point_of_no_return:1;
> > > + point_of_no_return:1,
> > > + /*
> > > + * Set by user space to check executability according to the
> > > + * caller's environment.
> > > + */
> > > + is_check:1;
> > > struct file *executable; /* Executable to pass to the interpreter */
> > > struct file *interpreter;
> > > struct file *file;
> > > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > > index 87e2dec79fea..2e87f2e3a79f 100644
> > > --- a/include/uapi/linux/fcntl.h
> > > +++ b/include/uapi/linux/fcntl.h
> > > @@ -154,6 +154,10 @@
> > > usable with open_by_handle_at(2). */
> > > #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
> > >
> > > +/* Flags for execveat2(2). */
> > > +#define AT_EXECVE_CHECK 0x10000 /* Only perform a check if execution
> > > + would be allowed. */
> > > +
> > > #if defined(__KERNEL__)
> > > #define AT_GETATTR_NOSEC 0x80000000
> > > #endif
> > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > index a60d2840559e..8ebdabd2ab81 100644
> > > --- a/kernel/audit.h
> > > +++ b/kernel/audit.h
> > > @@ -197,6 +197,7 @@ struct audit_context {
> > > struct open_how openat2;
> > > struct {
> > > int argc;
> > > + bool is_check;
> > > } execve;
> > > struct {
> > > char *name;
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index cd57053b4a69..8d9ba5600cf2 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -2662,6 +2662,7 @@ void __audit_bprm(struct linux_binprm *bprm)
> > >
> > > context->type = AUDIT_EXECVE;
> > > context->execve.argc = bprm->argc;
> > > + context->execve.is_check = bprm->is_check;
> > Where is execve.is_check used ?
>
> It is used in bprm_execve(), exposed to the audit framework, and
> potentially used by LSMs.
>
bprm_execve() uses bprm->is_check, not the context->execve.is_check.
> >
> >
> > > }
> > >
> > >
> > > diff --git a/security/security.c b/security/security.c
> > > index c5981e558bc2..456361ec249d 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -1249,6 +1249,12 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> > > * to 1 if AT_SECURE should be set to request libc enable secure mode. @bprm
> > > * contains the linux_binprm structure.
> > > *
> > > + * If execveat(2) is called with the AT_EXECVE_CHECK flag, bprm->is_check is
> > > + * set. The result must be the same as without this flag even if the execution
> > > + * will never really happen and @bprm will always be dropped.
> > > + *
> > > + * This hook must not change current->cred, only @bprm->cred.
> > > + *
> > > * Return: Returns 0 if the hook is successful and permission is granted.
> > > */
> > > int security_bprm_creds_for_exec(struct linux_binprm *bprm)
> > > @@ -3100,6 +3106,10 @@ int security_file_receive(struct file *file)
> > > * Save open-time permission checking state for later use upon file_permission,
> > > * and recheck access if anything has changed since inode_permission.
> > > *
> > > + * We can check if a file is opened for execution (e.g. execve(2) call), either
> > > + * directly or indirectly (e.g. ELF's ld.so) by checking file->f_flags &
> > > + * __FMODE_EXEC .
> > > + *
> > > * Return: Returns 0 if permission is granted.
> > > */
> > > int security_file_open(struct file *file)
> > > --
> > > 2.47.0
> > >
> > >
^ permalink raw reply
* Re: [PATCH v21 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
From: Mickaël Salaün @ 2024-11-20 9:42 UTC (permalink / raw)
To: Jeff Xu
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Theodore Ts'o,
Thibaut Sautereau, Vincent Strubel, Xiaoming Ni, Yin Fengwei,
kernel-hardening, linux-api, linux-fsdevel, linux-integrity,
linux-kernel, linux-security-module, Andy Lutomirski
In-Reply-To: <CABi2SkWgSOx__YafOC1NXxsxLJim4dGALthy5uCFnSCrzQ4zkA@mail.gmail.com>
On Tue, Nov 19, 2024 at 05:30:13PM -0800, Jeff Xu wrote:
> On Tue, Nov 12, 2024 at 12:06 PM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > The new SECBIT_EXEC_RESTRICT_FILE, SECBIT_EXEC_DENY_INTERACTIVE, and
> > their *_LOCKED counterparts are designed to be set by processes setting
> > up an execution environment, such as a user session, a container, or a
> > security sandbox. Unlike other securebits, these ones can be set by
> > unprivileged processes. Like seccomp filters or Landlock domains, the
> > securebits are inherited across processes.
> >
> > When SECBIT_EXEC_RESTRICT_FILE is set, programs interpreting code should
> > control executable resources according to execveat(2) + AT_EXECVE_CHECK
> > (see previous commit).
> >
> > When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should deny
> > execution of user interactive commands (which excludes executable
> > regular files).
> >
> > Being able to configure each of these securebits enables system
> > administrators or owner of image containers to gradually validate the
> > related changes and to identify potential issues (e.g. with interpreter
> > or audit logs).
> >
> > It should be noted that unlike other security bits, the
> > SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE bits are
> > dedicated to user space willing to restrict itself. Because of that,
> > they only make sense in the context of a trusted environment (e.g.
> > sandbox, container, user session, full system) where the process
> > changing its behavior (according to these bits) and all its parent
> > processes are trusted. Otherwise, any parent process could just execute
> > its own malicious code (interpreting a script or not), or even enforce a
> > seccomp filter to mask these bits.
> >
> > Such a secure environment can be achieved with an appropriate access
> > control (e.g. mount's noexec option, file access rights, LSM policy) and
> > an enlighten ld.so checking that libraries are allowed for execution
> > e.g., to protect against illegitimate use of LD_PRELOAD.
> >
> > Ptrace restrictions according to these securebits would not make sense
> > because of the processes' trust assumption.
> >
> > Scripts may need some changes to deal with untrusted data (e.g. stdin,
> > environment variables), but that is outside the scope of the kernel.
> >
> > See chromeOS's documentation about script execution control and the
> > related threat model:
> > https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
> >
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Link: https://lore.kernel.org/r/20241112191858.162021-3-mic@digikod.net
> > ---
> >
> > Changes since v20:
> > * Move UAPI documentation to a dedicated RST file and format it.
> >
> > Changes since v19:
> > * Replace SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT with
> > SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE:
> > https://lore.kernel.org/all/20240710.eiKohpa4Phai@digikod.net/
> > * Remove the ptrace restrictions, suggested by Andy.
> > * Improve documentation according to the discussion with Jeff.
> >
> > New design since v18:
> > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > ---
> > Documentation/userspace-api/check_exec.rst | 97 ++++++++++++++++++++++
> > include/uapi/linux/securebits.h | 24 +++++-
> > security/commoncap.c | 29 +++++--
> > 3 files changed, 143 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/userspace-api/check_exec.rst b/Documentation/userspace-api/check_exec.rst
> > index ad1aeaa5f6c0..1df5c7534af9 100644
> > --- a/Documentation/userspace-api/check_exec.rst
> > +++ b/Documentation/userspace-api/check_exec.rst
> > @@ -2,6 +2,21 @@
> > Executability check
> > ===================
> >
> > +The ``AT_EXECVE_CHECK`` :manpage:`execveat(2)` flag, and the
> > +``SECBIT_EXEC_RESTRICT_FILE`` and ``SECBIT_EXEC_DENY_INTERACTIVE`` securebits
> > +are intended for script interpreters and dynamic linkers to enforce a
> > +consistent execution security policy handled by the kernel. See the
> > +`samples/check-exec/inc.c`_ example.
> > +
> > +Whether an interpreter should check these securebits or not depends on the
> > +security risk of running malicious scripts with respect to the execution
> > +environment, and whether the kernel can check if a script is trustworthy or
> > +not. For instance, Python scripts running on a server can use arbitrary
> > +syscalls and access arbitrary files. Such interpreters should then be
> > +enlighten to use these securebits and let users define their security policy.
> > +However, a JavaScript engine running in a web browser should already be
> > +sandboxed and then should not be able to harm the user's environment.
> > +
> > AT_EXECVE_CHECK
> > ===============
> >
> > @@ -32,3 +47,85 @@ be executable, which also requires integrity guarantees.
> > To avoid race conditions leading to time-of-check to time-of-use issues,
> > ``AT_EXECVE_CHECK`` should be used with ``AT_EMPTY_PATH`` to check against a
> > file descriptor instead of a path.
> > +
> > +SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE
> > +==========================================================
> > +
> > +When ``SECBIT_EXEC_RESTRICT_FILE`` is set, a process should only interpret or
> > +execute a file if a call to :manpage:`execveat(2)` with the related file
> > +descriptor and the ``AT_EXECVE_CHECK`` flag succeed.
> > +
> > +This secure bit may be set by user session managers, service managers,
> > +container runtimes, sandboxer tools... Except for test environments, the
> > +related ``SECBIT_EXEC_RESTRICT_FILE_LOCKED`` bit should also be set.
> > +
> > +Programs should only enforce consistent restrictions according to the
> > +securebits but without relying on any other user-controlled configuration.
> > +Indeed, the use case for these securebits is to only trust executable code
> > +vetted by the system configuration (through the kernel), so we should be
> > +careful to not let untrusted users control this configuration.
> > +
> > +However, script interpreters may still use user configuration such as
> > +environment variables as long as it is not a way to disable the securebits
> > +checks. For instance, the ``PATH`` and ``LD_PRELOAD`` variables can be set by
> > +a script's caller. Changing these variables may lead to unintended code
> > +executions, but only from vetted executable programs, which is OK. For this to
> > +make sense, the system should provide a consistent security policy to avoid
> > +arbitrary code execution e.g., by enforcing a write xor execute policy.
> > +
> > +When ``SECBIT_EXEC_DENY_INTERACTIVE`` is set, a process should never interpret
> > +interactive user commands (e.g. scripts). However, if such commands are passed
> > +through a file descriptor (e.g. stdin), its content should be interpreted if a
> > +call to :manpage:`execveat(2)` with the related file descriptor and the
> > +``AT_EXECVE_CHECK`` flag succeed.
> > +
> > +For instance, script interpreters called with a script snippet as argument
> > +should always deny such execution if ``SECBIT_EXEC_DENY_INTERACTIVE`` is set.
> > +
> > +This secure bit may be set by user session managers, service managers,
> > +container runtimes, sandboxer tools... Except for test environments, the
> > +related ``SECBIT_EXEC_DENY_INTERACTIVE_LOCKED`` bit should also be set.
> > +
> > +Here is the expected behavior for a script interpreter according to combination
> > +of any exec securebits:
> > +
> > +1. ``SECBIT_EXEC_RESTRICT_FILE=0`` and ``SECBIT_EXEC_DENY_INTERACTIVE=0``
> > +
> > + Always interpret scripts, and allow arbitrary user commands (default).
> > +
> > + No threat, everyone and everything is trusted, but we can get ahead of
> > + potential issues thanks to the call to :manpage:`execveat(2)` with
> > + ``AT_EXECVE_CHECK`` which should always be performed but ignored by the
> > + script interpreter. Indeed, this check is still important to enable systems
> > + administrators to verify requests (e.g. with audit) and prepare for
> > + migration to a secure mode.
> > +
> > +2. ``SECBIT_EXEC_RESTRICT_FILE=1`` and ``SECBIT_EXEC_DENY_INTERACTIVE=0``
> > +
> > + Deny script interpretation if they are not executable, but allow
> > + arbitrary user commands.
> > +
> > + The threat is (potential) malicious scripts run by trusted (and not fooled)
> > + users. That can protect against unintended script executions (e.g. ``sh
> > + /tmp/*.sh``). This makes sense for (semi-restricted) user sessions.
> > +
> > +3. ``SECBIT_EXEC_RESTRICT_FILE=0`` and ``SECBIT_EXEC_DENY_INTERACTIVE=1``
> > +
> > + Always interpret scripts, but deny arbitrary user commands.
> > +
> > + This use case may be useful for secure services (i.e. without interactive
> > + user session) where scripts' integrity is verified (e.g. with IMA/EVM or
> > + dm-verity/IPE) but where access rights might not be ready yet. Indeed,
> > + arbitrary interactive commands would be much more difficult to check.
> > +
> > +4. ``SECBIT_EXEC_RESTRICT_FILE=1`` and ``SECBIT_EXEC_DENY_INTERACTIVE=1``
> > +
> > + Deny script interpretation if they are not executable, and also deny
> > + any arbitrary user commands.
> > +
> > + The threat is malicious scripts run by untrusted users (but trusted code).
> > + This makes sense for system services that may only execute trusted scripts.
> > +
> > +.. Links
> > +.. _samples/check-exec/inc.c:
> > + https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/check-exec/inc.c
> > diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
> > index d6d98877ff1a..3fba30dbd68b 100644
> > --- a/include/uapi/linux/securebits.h
> > +++ b/include/uapi/linux/securebits.h
> > @@ -52,10 +52,32 @@
> > #define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
> > (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
> >
> > +/* See Documentation/userspace-api/check_exec.rst */
> > +#define SECURE_EXEC_RESTRICT_FILE 8
> > +#define SECURE_EXEC_RESTRICT_FILE_LOCKED 9 /* make bit-8 immutable */
> > +
> > +#define SECBIT_EXEC_RESTRICT_FILE (issecure_mask(SECURE_EXEC_RESTRICT_FILE))
> > +#define SECBIT_EXEC_RESTRICT_FILE_LOCKED \
> > + (issecure_mask(SECURE_EXEC_RESTRICT_FILE_LOCKED))
> > +
> > +/* See Documentation/userspace-api/check_exec.rst */
> > +#define SECURE_EXEC_DENY_INTERACTIVE 10
> > +#define SECURE_EXEC_DENY_INTERACTIVE_LOCKED 11 /* make bit-10 immutable */
> > +
> > +#define SECBIT_EXEC_DENY_INTERACTIVE \
> > + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > +#define SECBIT_EXEC_DENY_INTERACTIVE_LOCKED \
> > + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE_LOCKED))
> > +
> > #define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
> > issecure_mask(SECURE_NO_SETUID_FIXUP) | \
> > issecure_mask(SECURE_KEEP_CAPS) | \
> > - issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
> > + issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
> > + issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> > + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > #define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
> >
> > +#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> > + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > +
> > #endif /* _UAPI_LINUX_SECUREBITS_H */
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index cefad323a0b1..52ea01acb453 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -1302,21 +1302,38 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> > & (old->securebits ^ arg2)) /*[1]*/
> > || ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
> > || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
> > - || (cap_capable(current_cred(),
> > - current_cred()->user_ns,
> > - CAP_SETPCAP,
> > - CAP_OPT_NONE) != 0) /*[4]*/
> > /*
> > * [1] no changing of bits that are locked
> > * [2] no unlocking of locks
> > * [3] no setting of unsupported bits
> > - * [4] doing anything requires privilege (go read about
> > - * the "sendmail capabilities bug")
> > */
> > )
> > /* cannot change a locked bit */
> > return -EPERM;
> >
> > + /*
> > + * Doing anything requires privilege (go read about the
> > + * "sendmail capabilities bug"), except for unprivileged bits.
> > + * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
> > + * restrictions enforced by the kernel but by user space on
> > + * itself.
> > + */
> > + if (cap_capable(current_cred(), current_cred()->user_ns,
> > + CAP_SETPCAP, CAP_OPT_NONE) != 0) {
> > + const unsigned long unpriv_and_locks =
> > + SECURE_ALL_UNPRIVILEGED |
> > + SECURE_ALL_UNPRIVILEGED << 1;
> > + const unsigned long changed = old->securebits ^ arg2;
> > +
> > + /* For legacy reason, denies non-change. */
> > + if (!changed)
> > + return -EPERM;
> > +
> > + /* Denies privileged changes. */
> > + if (changed & ~unpriv_and_locks)
> > + return -EPERM;
> > + }
> > +
> Is above a refactor (without functional change) or a bug fix ?
> maybe a separate commit with description ?
As explained in the comments this is a change to allow unprivileged
securebits to be set, which is related to the CAP_SETPCAP check and
required by this patch.
>
> > new = prepare_creds();
> > if (!new)
> > return -ENOMEM;
> > --
> > 2.47.0
> >
> >
^ permalink raw reply
* Re: [PATCH v21 1/6] exec: Add a new AT_EXECVE_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-11-20 9:42 UTC (permalink / raw)
To: Jeff Xu
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Theodore Ts'o,
Thibaut Sautereau, Vincent Strubel, Xiaoming Ni, Yin Fengwei,
kernel-hardening, linux-api, linux-fsdevel, linux-integrity,
linux-kernel, linux-security-module
In-Reply-To: <CABi2SkVRJC_7qoU56mDt3Ch7U9GnVeRogUt9wc9=32OtG6aatw@mail.gmail.com>
On Tue, Nov 19, 2024 at 05:17:00PM -0800, Jeff Xu wrote:
> On Tue, Nov 12, 2024 at 11:22 AM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > Add a new AT_EXECVE_CHECK flag to execveat(2) to check if a file would
> > be allowed for execution. The main use case is for script interpreters
> > and dynamic linkers to check execution permission according to the
> > kernel's security policy. Another use case is to add context to access
> > logs e.g., which script (instead of interpreter) accessed a file. As
> > any executable code, scripts could also use this check [1].
> >
> > This is different from faccessat(2) + X_OK which only checks a subset of
> > access rights (i.e. inode permission and mount options for regular
> > files), but not the full context (e.g. all LSM access checks). The main
> > use case for access(2) is for SUID processes to (partially) check access
> > on behalf of their caller. The main use case for execveat(2) +
> > AT_EXECVE_CHECK is to check if a script execution would be allowed,
> > according to all the different restrictions in place. Because the use
> > of AT_EXECVE_CHECK follows the exact kernel semantic as for a real
> > execution, user space gets the same error codes.
> >
> > An interesting point of using execveat(2) instead of openat2(2) is that
> > it decouples the check from the enforcement. Indeed, the security check
> > can be logged (e.g. with audit) without blocking an execution
> > environment not yet ready to enforce a strict security policy.
> >
> > LSMs can control or log execution requests with
> > security_bprm_creds_for_exec(). However, to enforce a consistent and
> > complete access control (e.g. on binary's dependencies) LSMs should
> > restrict file executability, or mesure executed files, with
> > security_file_open() by checking file->f_flags & __FMODE_EXEC.
> >
> > Because AT_EXECVE_CHECK is dedicated to user space interpreters, it
> > doesn't make sense for the kernel to parse the checked files, look for
> > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > if the format is unknown. Because of that, security_bprm_check() is
> > never called when AT_EXECVE_CHECK is used.
> >
> > It should be noted that script interpreters cannot directly use
> > execveat(2) (without this new AT_EXECVE_CHECK flag) because this could
> > lead to unexpected behaviors e.g., `python script.sh` could lead to Bash
> > being executed to interpret the script. Unlike the kernel, script
> > interpreters may just interpret the shebang as a simple comment, which
> > should not change for backward compatibility reasons.
> >
> > Because scripts or libraries files might not currently have the
> > executable permission set, or because we might want specific users to be
> > allowed to run arbitrary scripts, the following patch provides a dynamic
> > configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> > SECBIT_EXEC_DENY_INTERACTIVE securebits.
> >
> > This is a redesign of the CLIP OS 4's O_MAYEXEC:
> > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > This patch has been used for more than a decade with customized script
> > interpreters. Some examples can be found here:
> > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> >
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> > Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Link: https://lore.kernel.org/r/20241112191858.162021-2-mic@digikod.net
> > ---
> >
> > Changes since v20:
> > * Rename AT_CHECK to AT_EXECVE_CHECK, requested by Amir Goldstein and
> > Serge Hallyn.
> > * Move the UAPI documentation to a dedicated RST file.
> > * Add Reviewed-by: Serge Hallyn
> >
> > Changes since v19:
> > * Remove mention of "role transition" as suggested by Andy.
> > * Highlight the difference between security_bprm_creds_for_exec() and
> > the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> > discussed with Jeff.
> > * Improve documentation both in UAPI comments and kernel comments
> > (requested by Kees).
> >
> > New design since v18:
> > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > ---
> > Documentation/userspace-api/check_exec.rst | 34 ++++++++++++++++++++++
> > Documentation/userspace-api/index.rst | 1 +
> > fs/exec.c | 20 +++++++++++--
> > include/linux/binfmts.h | 7 ++++-
> > include/uapi/linux/fcntl.h | 4 +++
> > kernel/audit.h | 1 +
> > kernel/auditsc.c | 1 +
> > security/security.c | 10 +++++++
> > 8 files changed, 75 insertions(+), 3 deletions(-)
> > create mode 100644 Documentation/userspace-api/check_exec.rst
> >
> > diff --git a/Documentation/userspace-api/check_exec.rst b/Documentation/userspace-api/check_exec.rst
> > new file mode 100644
> > index 000000000000..ad1aeaa5f6c0
> > --- /dev/null
> > +++ b/Documentation/userspace-api/check_exec.rst
> > @@ -0,0 +1,34 @@
> > +===================
> > +Executability check
> > +===================
> > +
> > +AT_EXECVE_CHECK
> > +===============
> > +
> > +Passing the ``AT_EXECVE_CHECK`` flag to :manpage:`execveat(2)` only performs a
> > +check on a regular file and returns 0 if execution of this file would be
> > +allowed, ignoring the file format and then the related interpreter dependencies
> > +(e.g. ELF libraries, script's shebang).
> > +
> > +Programs should always perform this check to apply kernel-level checks against
> > +files that are not directly executed by the kernel but passed to a user space
> > +interpreter instead. All files that contain executable code, from the point of
> > +view of the interpreter, should be checked. However the result of this check
> > +should only be enforced according to ``SECBIT_EXEC_RESTRICT_FILE`` or
> > +``SECBIT_EXEC_DENY_INTERACTIVE.``.
> Regarding "should only"
> Userspace (e.g. libc) could decide to enforce even when
> SECBIT_EXEC_RESTRICT_FILE=0), i.e. if it determines not-enforcing
> doesn't make sense.
User space is always in control, but I don't think it would be wise to
not follow the configuration securebits (in a generic system) because
this could result to unattended behaviors (I don't have a specific one
in mind but...). That being said, configuration and checks are
standalones and specific/tailored systems are free to do the checks they
want.
> When SECBIT_EXEC_RESTRICT_FILE=1, userspace is bound to enforce.
>
> > +
> > +The main purpose of this flag is to improve the security and consistency of an
> > +execution environment to ensure that direct file execution (e.g.
> > +``./script.sh``) and indirect file execution (e.g. ``sh script.sh``) lead to
> > +the same result. For instance, this can be used to check if a file is
> > +trustworthy according to the caller's environment.
> > +
> > +In a secure environment, libraries and any executable dependencies should also
> > +be checked. For instance, dynamic linking should make sure that all libraries
> > +are allowed for execution to avoid trivial bypass (e.g. using ``LD_PRELOAD``).
> > +For such secure execution environment to make sense, only trusted code should
> > +be executable, which also requires integrity guarantees.
> > +
> > +To avoid race conditions leading to time-of-check to time-of-use issues,
> > +``AT_EXECVE_CHECK`` should be used with ``AT_EMPTY_PATH`` to check against a
> > +file descriptor instead of a path.
> > diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
> > index 274cc7546efc..6272bcf11296 100644
> > --- a/Documentation/userspace-api/index.rst
> > +++ b/Documentation/userspace-api/index.rst
> > @@ -35,6 +35,7 @@ Security-related interfaces
> > mfd_noexec
> > spec_ctrl
> > tee
> > + check_exec
> >
> > Devices and I/O
> > ===============
> > diff --git a/fs/exec.c b/fs/exec.c
> > index 6c53920795c2..bb83b6a39530 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -891,7 +891,8 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > .lookup_flags = LOOKUP_FOLLOW,
> > };
> >
> > - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > + if ((flags &
> > + ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_EXECVE_CHECK)) != 0)
> > return ERR_PTR(-EINVAL);
> > if (flags & AT_SYMLINK_NOFOLLOW)
> > open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> > @@ -1545,6 +1546,21 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
> > }
> > bprm->interp = bprm->filename;
> >
> > + /*
> > + * At this point, security_file_open() has already been called (with
> > + * __FMODE_EXEC) and access control checks for AT_EXECVE_CHECK will
> > + * stop just after the security_bprm_creds_for_exec() call in
> > + * bprm_execve(). Indeed, the kernel should not try to parse the
> > + * content of the file with exec_binprm() nor change the calling
> > + * thread, which means that the following security functions will be
> > + * not called:
> > + * - security_bprm_check()
> > + * - security_bprm_creds_from_file()
> > + * - security_bprm_committing_creds()
> > + * - security_bprm_committed_creds()
> > + */
> > + bprm->is_check = !!(flags & AT_EXECVE_CHECK);
> > +
> > retval = bprm_mm_init(bprm);
> > if (!retval)
> > return bprm;
> > @@ -1839,7 +1855,7 @@ static int bprm_execve(struct linux_binprm *bprm)
> >
> > /* Set the unchanging part of bprm->cred */
> > retval = security_bprm_creds_for_exec(bprm);
> > - if (retval)
> > + if (retval || bprm->is_check)
> > goto out;
> >
> > retval = exec_binprm(bprm);
> > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> > index e6c00e860951..8ff0eb3644a1 100644
> > --- a/include/linux/binfmts.h
> > +++ b/include/linux/binfmts.h
> > @@ -42,7 +42,12 @@ struct linux_binprm {
> > * Set when errors can no longer be returned to the
> > * original userspace.
> > */
> > - point_of_no_return:1;
> > + point_of_no_return:1,
> > + /*
> > + * Set by user space to check executability according to the
> > + * caller's environment.
> > + */
> > + is_check:1;
> > struct file *executable; /* Executable to pass to the interpreter */
> > struct file *interpreter;
> > struct file *file;
> > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > index 87e2dec79fea..2e87f2e3a79f 100644
> > --- a/include/uapi/linux/fcntl.h
> > +++ b/include/uapi/linux/fcntl.h
> > @@ -154,6 +154,10 @@
> > usable with open_by_handle_at(2). */
> > #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
> >
> > +/* Flags for execveat2(2). */
> > +#define AT_EXECVE_CHECK 0x10000 /* Only perform a check if execution
> > + would be allowed. */
> > +
> > #if defined(__KERNEL__)
> > #define AT_GETATTR_NOSEC 0x80000000
> > #endif
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index a60d2840559e..8ebdabd2ab81 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -197,6 +197,7 @@ struct audit_context {
> > struct open_how openat2;
> > struct {
> > int argc;
> > + bool is_check;
> > } execve;
> > struct {
> > char *name;
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index cd57053b4a69..8d9ba5600cf2 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2662,6 +2662,7 @@ void __audit_bprm(struct linux_binprm *bprm)
> >
> > context->type = AUDIT_EXECVE;
> > context->execve.argc = bprm->argc;
> > + context->execve.is_check = bprm->is_check;
> Where is execve.is_check used ?
It is used in bprm_execve(), exposed to the audit framework, and
potentially used by LSMs.
>
>
> > }
> >
> >
> > diff --git a/security/security.c b/security/security.c
> > index c5981e558bc2..456361ec249d 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1249,6 +1249,12 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> > * to 1 if AT_SECURE should be set to request libc enable secure mode. @bprm
> > * contains the linux_binprm structure.
> > *
> > + * If execveat(2) is called with the AT_EXECVE_CHECK flag, bprm->is_check is
> > + * set. The result must be the same as without this flag even if the execution
> > + * will never really happen and @bprm will always be dropped.
> > + *
> > + * This hook must not change current->cred, only @bprm->cred.
> > + *
> > * Return: Returns 0 if the hook is successful and permission is granted.
> > */
> > int security_bprm_creds_for_exec(struct linux_binprm *bprm)
> > @@ -3100,6 +3106,10 @@ int security_file_receive(struct file *file)
> > * Save open-time permission checking state for later use upon file_permission,
> > * and recheck access if anything has changed since inode_permission.
> > *
> > + * We can check if a file is opened for execution (e.g. execve(2) call), either
> > + * directly or indirectly (e.g. ELF's ld.so) by checking file->f_flags &
> > + * __FMODE_EXEC .
> > + *
> > * Return: Returns 0 if permission is granted.
> > */
> > int security_file_open(struct file *file)
> > --
> > 2.47.0
> >
> >
^ permalink raw reply
* Re: [PATCH v6 02/15] module: Introduce ksys_finit_module()
From: Roberto Sassu @ 2024-11-20 9:18 UTC (permalink / raw)
To: Luis Chamberlain, Christoph Hellwig
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, petr.pavlu,
samitolvanen, da.gomez, akpm, paul, jmorris, serge, shuah,
mcoquelin.stm32, alexandre.torgue, linux-integrity, linux-doc,
linux-kernel, linux-api, linux-modules, linux-security-module,
linux-kselftest, wufan, pbrobinson, zbyszek, mjg59, pmatilai,
jannh, dhowells, jikos, mkoutny, ppavlu, petr.vorel, mzerqung,
kgold, Roberto Sassu
In-Reply-To: <70952351d25817211509bf1cf43d3e665aef1481.camel@huaweicloud.com>
On Wed, 2024-11-20 at 10:16 +0100, Roberto Sassu wrote:
> On Tue, 2024-11-19 at 12:10 -0800, Luis Chamberlain wrote:
> > On Tue, Nov 19, 2024 at 01:14:02PM +0100, Christoph Hellwig wrote:
> > > On Tue, Nov 19, 2024 at 11:49:09AM +0100, Roberto Sassu wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > >
> > > > Introduce ksys_finit_module() to let kernel components request a kernel
> > > > module without requiring running modprobe.
> > >
> > > That does sound more than sketchy, even more so because the commit log
> > > completely fails to explain why you'd need to do that.
> >
> > I also don't think the commit log is correct, I don't see how the
> > code is preventing calling modprobe, the indepotent check is intended
> > to prevent duplicate module init calls which may allocate extra vmalloc
> > space only to release it. You can test to see if your patch has any
> > improvments by enabling MODULE_STATS and MODULE_DEBUG_AUTOLOAD_DUPS
> > and check before / after results of /sys/kernel/debug/modules/stats ,
> > right now this patch and commit log is not telling me anything useful.
>
> Maybe I misunderstood the code, but what causes modprobe to be executed
> in user space is a call to request_module().
>
> In my patch, I simply ported the code of the finit_module() system call
> to _ksys_finit_module(), net the conversion from struct fd to struct
> file, which is kept in the system call code.
>
> Also, from the kernel side, I'm providing a valid address for module
> arguments, and duplicating the string either with kmemdup() or
> strndup_user() in load_module(), depending on where the memory belongs
> to.
>
> Again, maybe I misunderstood, but I'm not introducing any functional
> change to the current behavior, the kernel side also provides a file
> descriptor and module arguments as user space would do (e.g. by
> executing insmod).
>
> As for the motivation, please have a look at my response to Christian:
Christoph, of course.
Roberto
> https://lore.kernel.org/linux-integrity/ZzzvAPetAn7CUEvx@bombadil.infradead.org/T/#ma8656b921bb5bfb60e7f10331061d462a87ce9f4
>
>
> In addition, you could also see how ksys_finit_module() is used here:
>
> https://lore.kernel.org/linux-integrity/20241119104922.2772571-8-roberto.sassu@huaweicloud.com/
>
> Thanks
>
> Roberto
^ permalink raw reply
* Re: [PATCH v6 02/15] module: Introduce ksys_finit_module()
From: Roberto Sassu @ 2024-11-20 9:16 UTC (permalink / raw)
To: Luis Chamberlain, Christoph Hellwig
Cc: zohar, dmitry.kasatkin, eric.snowberg, corbet, petr.pavlu,
samitolvanen, da.gomez, akpm, paul, jmorris, serge, shuah,
mcoquelin.stm32, alexandre.torgue, linux-integrity, linux-doc,
linux-kernel, linux-api, linux-modules, linux-security-module,
linux-kselftest, wufan, pbrobinson, zbyszek, mjg59, pmatilai,
jannh, dhowells, jikos, mkoutny, ppavlu, petr.vorel, mzerqung,
kgold, Roberto Sassu
In-Reply-To: <ZzzwxdHbG9HynADT@bombadil.infradead.org>
On Tue, 2024-11-19 at 12:10 -0800, Luis Chamberlain wrote:
> On Tue, Nov 19, 2024 at 01:14:02PM +0100, Christoph Hellwig wrote:
> > On Tue, Nov 19, 2024 at 11:49:09AM +0100, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > Introduce ksys_finit_module() to let kernel components request a kernel
> > > module without requiring running modprobe.
> >
> > That does sound more than sketchy, even more so because the commit log
> > completely fails to explain why you'd need to do that.
>
> I also don't think the commit log is correct, I don't see how the
> code is preventing calling modprobe, the indepotent check is intended
> to prevent duplicate module init calls which may allocate extra vmalloc
> space only to release it. You can test to see if your patch has any
> improvments by enabling MODULE_STATS and MODULE_DEBUG_AUTOLOAD_DUPS
> and check before / after results of /sys/kernel/debug/modules/stats ,
> right now this patch and commit log is not telling me anything useful.
Maybe I misunderstood the code, but what causes modprobe to be executed
in user space is a call to request_module().
In my patch, I simply ported the code of the finit_module() system call
to _ksys_finit_module(), net the conversion from struct fd to struct
file, which is kept in the system call code.
Also, from the kernel side, I'm providing a valid address for module
arguments, and duplicating the string either with kmemdup() or
strndup_user() in load_module(), depending on where the memory belongs
to.
Again, maybe I misunderstood, but I'm not introducing any functional
change to the current behavior, the kernel side also provides a file
descriptor and module arguments as user space would do (e.g. by
executing insmod).
As for the motivation, please have a look at my response to Christian:
https://lore.kernel.org/linux-integrity/ZzzvAPetAn7CUEvx@bombadil.infradead.org/T/#ma8656b921bb5bfb60e7f10331061d462a87ce9f4
In addition, you could also see how ksys_finit_module() is used here:
https://lore.kernel.org/linux-integrity/20241119104922.2772571-8-roberto.sassu@huaweicloud.com/
Thanks
Roberto
^ permalink raw reply
* Re: [PATCH v21 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
From: Jeff Xu @ 2024-11-20 1:30 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Paul Moore, Serge Hallyn,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Linus Torvalds,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Theodore Ts'o,
Thibaut Sautereau, Vincent Strubel, Xiaoming Ni, Yin Fengwei,
kernel-hardening, linux-api, linux-fsdevel, linux-integrity,
linux-kernel, linux-security-module, Andy Lutomirski
In-Reply-To: <20241112191858.162021-3-mic@digikod.net>
On Tue, Nov 12, 2024 at 12:06 PM Mickaël Salaün <mic@digikod.net> wrote:
>
> The new SECBIT_EXEC_RESTRICT_FILE, SECBIT_EXEC_DENY_INTERACTIVE, and
> their *_LOCKED counterparts are designed to be set by processes setting
> up an execution environment, such as a user session, a container, or a
> security sandbox. Unlike other securebits, these ones can be set by
> unprivileged processes. Like seccomp filters or Landlock domains, the
> securebits are inherited across processes.
>
> When SECBIT_EXEC_RESTRICT_FILE is set, programs interpreting code should
> control executable resources according to execveat(2) + AT_EXECVE_CHECK
> (see previous commit).
>
> When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should deny
> execution of user interactive commands (which excludes executable
> regular files).
>
> Being able to configure each of these securebits enables system
> administrators or owner of image containers to gradually validate the
> related changes and to identify potential issues (e.g. with interpreter
> or audit logs).
>
> It should be noted that unlike other security bits, the
> SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE bits are
> dedicated to user space willing to restrict itself. Because of that,
> they only make sense in the context of a trusted environment (e.g.
> sandbox, container, user session, full system) where the process
> changing its behavior (according to these bits) and all its parent
> processes are trusted. Otherwise, any parent process could just execute
> its own malicious code (interpreting a script or not), or even enforce a
> seccomp filter to mask these bits.
>
> Such a secure environment can be achieved with an appropriate access
> control (e.g. mount's noexec option, file access rights, LSM policy) and
> an enlighten ld.so checking that libraries are allowed for execution
> e.g., to protect against illegitimate use of LD_PRELOAD.
>
> Ptrace restrictions according to these securebits would not make sense
> because of the processes' trust assumption.
>
> Scripts may need some changes to deal with untrusted data (e.g. stdin,
> environment variables), but that is outside the scope of the kernel.
>
> See chromeOS's documentation about script execution control and the
> related threat model:
> https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Reviewed-by: Serge Hallyn <serge@hallyn.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20241112191858.162021-3-mic@digikod.net
> ---
>
> Changes since v20:
> * Move UAPI documentation to a dedicated RST file and format it.
>
> Changes since v19:
> * Replace SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT with
> SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE:
> https://lore.kernel.org/all/20240710.eiKohpa4Phai@digikod.net/
> * Remove the ptrace restrictions, suggested by Andy.
> * Improve documentation according to the discussion with Jeff.
>
> New design since v18:
> https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> ---
> Documentation/userspace-api/check_exec.rst | 97 ++++++++++++++++++++++
> include/uapi/linux/securebits.h | 24 +++++-
> security/commoncap.c | 29 +++++--
> 3 files changed, 143 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/userspace-api/check_exec.rst b/Documentation/userspace-api/check_exec.rst
> index ad1aeaa5f6c0..1df5c7534af9 100644
> --- a/Documentation/userspace-api/check_exec.rst
> +++ b/Documentation/userspace-api/check_exec.rst
> @@ -2,6 +2,21 @@
> Executability check
> ===================
>
> +The ``AT_EXECVE_CHECK`` :manpage:`execveat(2)` flag, and the
> +``SECBIT_EXEC_RESTRICT_FILE`` and ``SECBIT_EXEC_DENY_INTERACTIVE`` securebits
> +are intended for script interpreters and dynamic linkers to enforce a
> +consistent execution security policy handled by the kernel. See the
> +`samples/check-exec/inc.c`_ example.
> +
> +Whether an interpreter should check these securebits or not depends on the
> +security risk of running malicious scripts with respect to the execution
> +environment, and whether the kernel can check if a script is trustworthy or
> +not. For instance, Python scripts running on a server can use arbitrary
> +syscalls and access arbitrary files. Such interpreters should then be
> +enlighten to use these securebits and let users define their security policy.
> +However, a JavaScript engine running in a web browser should already be
> +sandboxed and then should not be able to harm the user's environment.
> +
> AT_EXECVE_CHECK
> ===============
>
> @@ -32,3 +47,85 @@ be executable, which also requires integrity guarantees.
> To avoid race conditions leading to time-of-check to time-of-use issues,
> ``AT_EXECVE_CHECK`` should be used with ``AT_EMPTY_PATH`` to check against a
> file descriptor instead of a path.
> +
> +SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE
> +==========================================================
> +
> +When ``SECBIT_EXEC_RESTRICT_FILE`` is set, a process should only interpret or
> +execute a file if a call to :manpage:`execveat(2)` with the related file
> +descriptor and the ``AT_EXECVE_CHECK`` flag succeed.
> +
> +This secure bit may be set by user session managers, service managers,
> +container runtimes, sandboxer tools... Except for test environments, the
> +related ``SECBIT_EXEC_RESTRICT_FILE_LOCKED`` bit should also be set.
> +
> +Programs should only enforce consistent restrictions according to the
> +securebits but without relying on any other user-controlled configuration.
> +Indeed, the use case for these securebits is to only trust executable code
> +vetted by the system configuration (through the kernel), so we should be
> +careful to not let untrusted users control this configuration.
> +
> +However, script interpreters may still use user configuration such as
> +environment variables as long as it is not a way to disable the securebits
> +checks. For instance, the ``PATH`` and ``LD_PRELOAD`` variables can be set by
> +a script's caller. Changing these variables may lead to unintended code
> +executions, but only from vetted executable programs, which is OK. For this to
> +make sense, the system should provide a consistent security policy to avoid
> +arbitrary code execution e.g., by enforcing a write xor execute policy.
> +
> +When ``SECBIT_EXEC_DENY_INTERACTIVE`` is set, a process should never interpret
> +interactive user commands (e.g. scripts). However, if such commands are passed
> +through a file descriptor (e.g. stdin), its content should be interpreted if a
> +call to :manpage:`execveat(2)` with the related file descriptor and the
> +``AT_EXECVE_CHECK`` flag succeed.
> +
> +For instance, script interpreters called with a script snippet as argument
> +should always deny such execution if ``SECBIT_EXEC_DENY_INTERACTIVE`` is set.
> +
> +This secure bit may be set by user session managers, service managers,
> +container runtimes, sandboxer tools... Except for test environments, the
> +related ``SECBIT_EXEC_DENY_INTERACTIVE_LOCKED`` bit should also be set.
> +
> +Here is the expected behavior for a script interpreter according to combination
> +of any exec securebits:
> +
> +1. ``SECBIT_EXEC_RESTRICT_FILE=0`` and ``SECBIT_EXEC_DENY_INTERACTIVE=0``
> +
> + Always interpret scripts, and allow arbitrary user commands (default).
> +
> + No threat, everyone and everything is trusted, but we can get ahead of
> + potential issues thanks to the call to :manpage:`execveat(2)` with
> + ``AT_EXECVE_CHECK`` which should always be performed but ignored by the
> + script interpreter. Indeed, this check is still important to enable systems
> + administrators to verify requests (e.g. with audit) and prepare for
> + migration to a secure mode.
> +
> +2. ``SECBIT_EXEC_RESTRICT_FILE=1`` and ``SECBIT_EXEC_DENY_INTERACTIVE=0``
> +
> + Deny script interpretation if they are not executable, but allow
> + arbitrary user commands.
> +
> + The threat is (potential) malicious scripts run by trusted (and not fooled)
> + users. That can protect against unintended script executions (e.g. ``sh
> + /tmp/*.sh``). This makes sense for (semi-restricted) user sessions.
> +
> +3. ``SECBIT_EXEC_RESTRICT_FILE=0`` and ``SECBIT_EXEC_DENY_INTERACTIVE=1``
> +
> + Always interpret scripts, but deny arbitrary user commands.
> +
> + This use case may be useful for secure services (i.e. without interactive
> + user session) where scripts' integrity is verified (e.g. with IMA/EVM or
> + dm-verity/IPE) but where access rights might not be ready yet. Indeed,
> + arbitrary interactive commands would be much more difficult to check.
> +
> +4. ``SECBIT_EXEC_RESTRICT_FILE=1`` and ``SECBIT_EXEC_DENY_INTERACTIVE=1``
> +
> + Deny script interpretation if they are not executable, and also deny
> + any arbitrary user commands.
> +
> + The threat is malicious scripts run by untrusted users (but trusted code).
> + This makes sense for system services that may only execute trusted scripts.
> +
> +.. Links
> +.. _samples/check-exec/inc.c:
> + https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/check-exec/inc.c
> diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
> index d6d98877ff1a..3fba30dbd68b 100644
> --- a/include/uapi/linux/securebits.h
> +++ b/include/uapi/linux/securebits.h
> @@ -52,10 +52,32 @@
> #define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
> (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
>
> +/* See Documentation/userspace-api/check_exec.rst */
> +#define SECURE_EXEC_RESTRICT_FILE 8
> +#define SECURE_EXEC_RESTRICT_FILE_LOCKED 9 /* make bit-8 immutable */
> +
> +#define SECBIT_EXEC_RESTRICT_FILE (issecure_mask(SECURE_EXEC_RESTRICT_FILE))
> +#define SECBIT_EXEC_RESTRICT_FILE_LOCKED \
> + (issecure_mask(SECURE_EXEC_RESTRICT_FILE_LOCKED))
> +
> +/* See Documentation/userspace-api/check_exec.rst */
> +#define SECURE_EXEC_DENY_INTERACTIVE 10
> +#define SECURE_EXEC_DENY_INTERACTIVE_LOCKED 11 /* make bit-10 immutable */
> +
> +#define SECBIT_EXEC_DENY_INTERACTIVE \
> + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> +#define SECBIT_EXEC_DENY_INTERACTIVE_LOCKED \
> + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE_LOCKED))
> +
> #define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
> issecure_mask(SECURE_NO_SETUID_FIXUP) | \
> issecure_mask(SECURE_KEEP_CAPS) | \
> - issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
> + issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
> + issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> #define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
>
> +#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> +
> #endif /* _UAPI_LINUX_SECUREBITS_H */
> diff --git a/security/commoncap.c b/security/commoncap.c
> index cefad323a0b1..52ea01acb453 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1302,21 +1302,38 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> & (old->securebits ^ arg2)) /*[1]*/
> || ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
> || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
> - || (cap_capable(current_cred(),
> - current_cred()->user_ns,
> - CAP_SETPCAP,
> - CAP_OPT_NONE) != 0) /*[4]*/
> /*
> * [1] no changing of bits that are locked
> * [2] no unlocking of locks
> * [3] no setting of unsupported bits
> - * [4] doing anything requires privilege (go read about
> - * the "sendmail capabilities bug")
> */
> )
> /* cannot change a locked bit */
> return -EPERM;
>
> + /*
> + * Doing anything requires privilege (go read about the
> + * "sendmail capabilities bug"), except for unprivileged bits.
> + * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
> + * restrictions enforced by the kernel but by user space on
> + * itself.
> + */
> + if (cap_capable(current_cred(), current_cred()->user_ns,
> + CAP_SETPCAP, CAP_OPT_NONE) != 0) {
> + const unsigned long unpriv_and_locks =
> + SECURE_ALL_UNPRIVILEGED |
> + SECURE_ALL_UNPRIVILEGED << 1;
> + const unsigned long changed = old->securebits ^ arg2;
> +
> + /* For legacy reason, denies non-change. */
> + if (!changed)
> + return -EPERM;
> +
> + /* Denies privileged changes. */
> + if (changed & ~unpriv_and_locks)
> + return -EPERM;
> + }
> +
Is above a refactor (without functional change) or a bug fix ?
maybe a separate commit with description ?
> new = prepare_creds();
> if (!new)
> return -ENOMEM;
> --
> 2.47.0
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox