sendmsg() with a single iov becomes ITER_UBUF, sendmsg() with multiple
iovs becomes ITER_IOVEC. Instead of adjusting the check to include
ITER_UBUF, drop the check completely. The callers are guaranteed
to happen from system call side and we don't need to pay runtime
cost to verify it.

Fixes: bd61848900bf ("net: devmem: Implement TX path")
Signed-off-by: Stanislav Fomichev <stfomichev@gmail.com>
---
 net/core/datagram.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/net/core/datagram.c b/net/core/datagram.c
index 9ef5442536f5..e04908276a32 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -702,13 +702,6 @@ zerocopy_fill_skb_from_devmem(struct sk_buff *skb, struct iov_iter *from,
 	size_t virt_addr, size, off;
 	struct net_iov *niov;
 
-	/* Devmem filling works by taking an IOVEC from the user where the
-	 * iov_addrs are interpreted as an offset in bytes into the dma-buf to
-	 * send from. We do not support other iter types.
-	 */
-	if (iov_iter_type(from) != ITER_IOVEC)
-		return -EFAULT;
-
 	while (length && iov_iter_count(from)) {
 		if (i == MAX_SKB_FRAGS)
 			return -EMSGSIZE;
-- 
2.49.0

On 5/16/25 23:54, Stanislav Fomichev wrote:
> sendmsg() with a single iov becomes ITER_UBUF, sendmsg() with multiple
> iovs becomes ITER_IOVEC. Instead of adjusting the check to include
> ITER_UBUF, drop the check completely. The callers are guaranteed
> to happen from system call side and we don't need to pay runtime
> cost to verify it.

I asked for this because io_uring can pass bvecs. Only sendzc can
pass that with cmsg, so probably you won't be able to hit any
real issue, but io_uring needs and soon will have bvec support for
normal sends as well. One can argue we should care as it isn't
merged yet, but there is something very very wrong if an unrelated
and legal io_uring change is able to open a vulnerability in the
devmem path.

On the bright side, checking UBUF on top doesn't add extra overhead
as compilers can put it under the same test and generate sth like:

if (iter->type > ITER_IOVEC) /* fail */

-- 
Pavel Begunkov

On 05/19, Pavel Begunkov wrote:
> On 5/16/25 23:54, Stanislav Fomichev wrote:
> > sendmsg() with a single iov becomes ITER_UBUF, sendmsg() with multiple
> > iovs becomes ITER_IOVEC. Instead of adjusting the check to include
> > ITER_UBUF, drop the check completely. The callers are guaranteed
> > to happen from system call side and we don't need to pay runtime
> > cost to verify it.
> 
> I asked for this because io_uring can pass bvecs. Only sendzc can
> pass that with cmsg, so probably you won't be able to hit any
> real issue, but io_uring needs and soon will have bvec support for
> normal sends as well. One can argue we should care as it isn't
> merged yet, but there is something very very wrong if an unrelated
> and legal io_uring change is able to open a vulnerability in the
> devmem path.

Any reason not to filter these out on the io_uring side? Or you'll
have to interpret sendmsg flags again which is not nice?

On 5/19/25 15:41, Stanislav Fomichev wrote:
> On 05/19, Pavel Begunkov wrote:
>> On 5/16/25 23:54, Stanislav Fomichev wrote:
>>> sendmsg() with a single iov becomes ITER_UBUF, sendmsg() with multiple
>>> iovs becomes ITER_IOVEC. Instead of adjusting the check to include
>>> ITER_UBUF, drop the check completely. The callers are guaranteed
>>> to happen from system call side and we don't need to pay runtime
>>> cost to verify it.
>>
>> I asked for this because io_uring can pass bvecs. Only sendzc can
>> pass that with cmsg, so probably you won't be able to hit any
>> real issue, but io_uring needs and soon will have bvec support for
>> normal sends as well. One can argue we should care as it isn't
>> merged yet, but there is something very very wrong if an unrelated
>> and legal io_uring change is able to open a vulnerability in the
>> devmem path.
> 
> Any reason not to filter these out on the io_uring side? Or you'll
> have to interpret sendmsg flags again which is not nice?

Right, io_uring would need to walk cmsg for all sends, which is not
great for layering. And then it's really a devmem quirk that it uses
iterators in a non orthodox way, it'd be awkward to check a random
devmem restriction in io_uring, when otherwise they know nothing
about each other. And it's safer to keep local to devmem, because
try to remember if something changes, and what if there is someone
new passing non-iovec iter + cmsg in the future.

-- 
Pavel Begunkov

On 05/19, Pavel Begunkov wrote:
> On 5/19/25 15:41, Stanislav Fomichev wrote:
> > On 05/19, Pavel Begunkov wrote:
> > > On 5/16/25 23:54, Stanislav Fomichev wrote:
> > > > sendmsg() with a single iov becomes ITER_UBUF, sendmsg() with multiple
> > > > iovs becomes ITER_IOVEC. Instead of adjusting the check to include
> > > > ITER_UBUF, drop the check completely. The callers are guaranteed
> > > > to happen from system call side and we don't need to pay runtime
> > > > cost to verify it.
> > > 
> > > I asked for this because io_uring can pass bvecs. Only sendzc can
> > > pass that with cmsg, so probably you won't be able to hit any
> > > real issue, but io_uring needs and soon will have bvec support for
> > > normal sends as well. One can argue we should care as it isn't
> > > merged yet, but there is something very very wrong if an unrelated
> > > and legal io_uring change is able to open a vulnerability in the
> > > devmem path.
> > 
> > Any reason not to filter these out on the io_uring side? Or you'll
> > have to interpret sendmsg flags again which is not nice?
> 
> Right, io_uring would need to walk cmsg for all sends, which is not
> great for layering. And then it's really a devmem quirk that it uses
> iterators in a non orthodox way, it'd be awkward to check a random
> devmem restriction in io_uring, when otherwise they know nothing
> about each other. And it's safer to keep local to devmem, because
> try to remember if something changes, and what if there is someone
> new passing non-iovec iter + cmsg in the future.

SG, will change this to filter both IOVEC and UBUF, thanks!
(pending discussion with Al about what to do with UBUF in
https://lore.kernel.org/netdev/20250517000907.GW2023217@ZenIV/)

On Fri, May 16, 2025 at 3:54 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> sendmsg() with a single iov becomes ITER_UBUF, sendmsg() with multiple
> iovs becomes ITER_IOVEC. Instead of adjusting the check to include
> ITER_UBUF, drop the check completely. The callers are guaranteed
> to happen from system call side and we don't need to pay runtime
> cost to verify it.
>
> Fixes: bd61848900bf ("net: devmem: Implement TX path")
> Signed-off-by: Stanislav Fomichev <stfomichev@gmail.com>

Looks good to me, but can we please bundle this with the fix for
ITER_UBUF, and if possible get some test coverage in ncdevmem?

On 05/16, Mina Almasry wrote:
> On Fri, May 16, 2025 at 3:54 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > sendmsg() with a single iov becomes ITER_UBUF, sendmsg() with multiple
> > iovs becomes ITER_IOVEC. Instead of adjusting the check to include
> > ITER_UBUF, drop the check completely. The callers are guaranteed
> > to happen from system call side and we don't need to pay runtime
> > cost to verify it.
> >
> > Fixes: bd61848900bf ("net: devmem: Implement TX path")
> > Signed-off-by: Stanislav Fomichev <stfomichev@gmail.com>
> 
> Looks good to me, but can we please bundle this with the fix for
> ITER_UBUF, and if possible get some test coverage in ncdevmem?

Yeah, let's see where we arrive at wrt iter_iov_len with Al.
Will repost with a selftest change.. Thanks!

---
pw-bot: cr