Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley
        Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United
        Kingdom.
        Registered in England and Wales under Company Registration No. 3798903
From:   David Howells <dhowells@redhat.com>
In-Reply-To: <b0a0cb0fac4ebdc23f01d183a9de10731dc90093.camel@redhat.com>
References: <b0a0cb0fac4ebdc23f01d183a9de10731dc90093.camel@redhat.com> <3112097.1687814081@warthog.procyon.org.uk> <20230626142257.6e14a801@kernel.org>
To:     Paolo Abeni <pabeni@redhat.com>
Cc:     dhowells@redhat.com, Jakub Kicinski <kuba@kernel.org>,
        Ilya Dryomov <idryomov@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>, ceph-devel@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: Is ->sendmsg() allowed to change the msghdr struct it is given?
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3132609.1687871361.1@warthog.procyon.org.uk>
Date:   Tue, 27 Jun 2023 14:09:21 +0100
Message-ID: <3132610.1687871361@warthog.procyon.org.uk>
Precedence: bulk

Paolo Abeni <pabeni@redhat.com> wrote:

> udp_sendmsg() can set the MSG_TRUNC bit in msg->msg_flags, so I guess
> that kind of actions are sort of allowed. Still, AFAICS, the kernel
> based msghdr is not copied back to the user-space, so such change
> should be almost a no-op in practice.
> 
> @David: which would be the end goal for such action?

Various places in the kernel use sock_sendmsg() - and I've added a bunch more
with the MSG_SPLICE_PAGES patches.  For some of the things I've added, there's
a loop which used to call ->sendpage() and now calls sock_sendmsg().  In most
of those places, msghdr will get reset each time round the loop - but not in
all cases.

Of particular immediate interest is net/ceph/messenger_v2.c.  If you go to:

	https://lore.kernel.org/r/3111635.1687813501@warthog.procyon.org.uk/

and look at the resultant code:

	static int do_sendmsg(struct socket *sock, struct iov_iter *it)
	{
		struct msghdr msg = { .msg_flags = CEPH_MSG_FLAGS };
		int ret;

		msg.msg_iter = *it;
		while (iov_iter_count(it)) {
			ret = sock_sendmsg(sock, &msg);
			if (ret <= 0) {
				if (ret == -EAGAIN)
					ret = 0;
				return ret;
			}

			iov_iter_advance(it, ret);
		}

		WARN_ON(msg_data_left(&msg));
		return 1;
	}

for example.  It could/would malfunction if sendmsg() is allowed to modify
msghdr - or if it doesn't update msg_iter.  Likewise:

	static int do_try_sendpage(struct socket *sock, struct iov_iter *it)
	{
		struct msghdr msg = { .msg_flags = CEPH_MSG_FLAGS };
		struct bio_vec bv;
		int ret;

		if (WARN_ON(!iov_iter_is_bvec(it)))
			return -EINVAL;

		while (iov_iter_count(it)) {
			/* iov_iter_iovec() for ITER_BVEC */
			bvec_set_page(&bv, it->bvec->bv_page,
				      min(iov_iter_count(it),
					  it->bvec->bv_len - it->iov_offset),
				      it->bvec->bv_offset + it->iov_offset);

			/*
			 * MSG_SPLICE_PAGES cannot properly handle pages with
			 * page_count == 0, we need to fall back to sendmsg if
			 * that's the case.
			 *
			 * Same goes for slab pages: skb_can_coalesce() allows
			 * coalescing neighboring slab objects into a single frag
			 * which triggers one of hardened usercopy checks.
			 */
			if (sendpage_ok(bv.bv_page))
				msg.msg_flags |= MSG_SPLICE_PAGES;
			else
				msg.msg_flags &= ~MSG_SPLICE_PAGES;

			iov_iter_bvec(&msg.msg_iter, ITER_SOURCE, &bv, 1, bv.bv_len);
			ret = sock_sendmsg(sock, &msg);
			if (ret <= 0) {
				if (ret == -EAGAIN)
					ret = 0;
				return ret;
			}

			iov_iter_advance(it, ret);
		}

		return 1;
	}

could be similarly affected if ->sendmsg() mucks about with msg_flags.

David