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

From: Guenter Roeck <linux@roeck-us.net>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "menglong8.dong@gmail.com" <menglong8.dong@gmail.com>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
	"herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>,
	"dong.menglong@zte.com.cn" <dong.menglong@zte.com.cn>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
Date: Tue, 16 Mar 2021 18:37:58 -0700	[thread overview]
Message-ID: <20210317013758.GA134033@roeck-us.net> (raw)
In-Reply-To: <CAHp75VdE3fkCjb53vBso5uJX9aEFtAOAdh5NVOSbK0YR64+jOg@mail.gmail.com>

On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
> On Wednesday, March 17, 2021, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > Hi,
> >
> > On Tue, Mar 09, 2021 at 05:51:35PM -0800, menglong8.dong@gmail.com wrote:
> > > From: Menglong Dong <dong.menglong@zte.com.cn>
> > >
> > > The bit mask for MSG_* seems a little confused here. Replace it
> > > with BIT() to make it clear to understand.
> > >
> > > Signed-off-by: Menglong Dong <dong.menglong@zte.com.cn>
> >
> > I must admit that I am a bit puzzled,
> 
> 
> I have checked the values and don’t see a problem. So, the only difference
> is the type int vs. unsigned long. I think this simply reveals an issue
> somewhere in the code.
> 
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