Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*

On Wed, Mar 17, 2021 at 10:21 AM Menglong Dong [off-list ref] wrote:

On Wed, Mar 17, 2021 at 9:38 AM Guenter Roeck [off-list ref] wrote:

quoted

On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:

quoted

On Wednesday, March 17, 2021, Guenter Roeck [off-list ref] wrote:

...

quoted

The problem is in net/packet/af_packet.c:packet_recvmsg(). This function,
as well as all other rcvmsg functions, is declared as

static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
                          int flags)

MSG_CMSG_COMPAT (0x80000000) is set in flags, meaning its value is negative.
This is then evaluated in

       if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE))
                goto out;

If any of those flags is declared as BIT() and thus long, flags is
sign-extended to long. Since it is negative, its upper 32 bits will be set,
the if statement evaluates as true, and the function bails out.

This is relatively easy to fix here with, for example,

        if ((unsigned int)flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE))
                goto out;

but that is just a hack, and it doesn't solve the real problem:
Each function in struct proto_ops which passes flags passes it as int
(see include/linux/net.h:struct proto_ops). Each such function, if
called with MSG_CMSG_COMPAT set, will fail a match against
~(MSG_anything) if MSG_anything is declared as BIT() or long.

As it turns out, I was kind of lucky to catch the problem: So far I have
seen it only on mips64 kernels with N32 userspace.

Guenter

 Wow, now the usages of 'msg_flag' really puzzle me. Seems that
it is used as 'unsigned int' somewhere, but 'int' somewhere
else.

As I found, It is used as 'int' in 'netlink_recvmsg()',
'io_sr_msg->msg_flags', 'atalk_sendmsg()',
'dn_recvmsg()',  'proto_ops->recvmsg()', etc.

So what should I do? Revert this patch? Or fix the usages of 'flags'?
Or change the type of MSG_* to 'unsigned int'? I prefer the last
one(the usages of 'flags' can be fixed too, maybe later).

The problematic code is negation of the flags when it's done in
operations like &.
It maybe fixed by swapping positions of the arguments, i.e. ~(FOO |
BAR) & flags.

All this is a beast called "integer promotions" in the C standard.

The best is to try to get flags to be unsigned. By how invasive it may be?

-- 
With Best Regards,
Andy Shevchenko