On Mon, Feb 28, 2022 at 3:27 PM Harold Huang [off-list ref] wrote:

Thanks for the suggestions.

On Mon, Feb 28, 2022 at 1:17 PM Jason Wang [off-list ref] wrote:

quoted

On Mon, Feb 28, 2022 at 12:59 PM Eric Dumazet [off-list ref] wrote:

quoted

On Sun, Feb 27, 2022 at 8:20 PM Jason Wang [off-list ref] wrote:

quoted

On Mon, Feb 28, 2022 at 12:06 PM Eric Dumazet [off-list ref] wrote:

quoted

How big n can be ?

BTW I could not find where m->msg_controllen was checked in tun_sendmsg().

struct tun_msg_ctl *ctl = m->msg_control;

if (ctl && (ctl->type == TUN_MSG_PTR)) {

     int n = ctl->num;  // can be set to values in [0..65535]

     for (i = 0; i < n; i++) {

         xdp = &((struct xdp_buff *)ctl->ptr)[i];


I really do not understand how we prevent malicious user space from
crashing the kernel.

It looks to me the only user for this is vhost-net which limits it to
64, userspace can't use sendmsg() directly on tap.

Ah right, thanks for the clarification.

(IMO, either remove the "msg.msg_controllen = sizeof(ctl);" from handle_tx_zerocopy(), or add sanity checks in tun_sendmsg())

Right, Harold, want to do that?

I am greatly willing to do that. But  I am not quite sure about this.

If we remove the "msg.msg_controllen = sizeof(ctl);" from
handle_tx_zerocopy(), it seems msg.msg_controllen is always 0. What
does it stands for?

It means msg_controllen is not used. But see below (adding sanity
check seems to be better).

I see tap_sendmsg in drivers/net/tap.c also uses msg_controller to
send batched xdp buffers. Do we need to add similar sanity checks to
tap_sendmsg  as tun_sendmsg?

I think the point is to make sure the caller doesn't send us too short
msg_control. E.g the msg_controllen should be sizeof(tun_msg_ctl).

So we probably need to check in both places. (And initialize
msg_controllen is vhost_tx_batch())

Thanks