netdev - Re: [PATCH v2 net-next 2/2] net: socket: change MSG_CMSG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sun, 21 Mar 2021 09:20:43 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     menglong8.dong@...il.com, herbert@...dor.apana.org.au,
        andy.shevchenko@...il.com, kuba@...nel.org, David.Laight@...lab.com
Cc:     davem@...emloft.net, dong.menglong@....com.cn,
        viro@...iv.linux.org.uk, axboe@...nel.dk,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 2/2] net: socket: change MSG_CMSG_COMPAT to
 BIT(21)

On 3/21/21 6:43 AM, menglong8.dong@...il.com wrote:
> From: Menglong Dong <dong.menglong@....com.cn>
> 
> Currently, MSG_CMSG_COMPAT is defined as '1 << 31'. However, 'msg_flags'
> is defined with type of 'int' somewhere, such as 'packet_recvmsg' and
> other recvmsg functions:
> 
> static int packet_recvmsg(struct socket *sock, struct msghdr *msg,
> 			  size_t len,
> 			  int flags)
> 
> If MSG_CMSG_COMPAT is set in 'flags', it's value will be negative.
> Once it perform bit operations with MSG_*, the upper 32 bits of
> the result will be set, just like what Guenter Roeck explained
> here:
> 
> https://lore.kernel.org/netdev/20210317013758.GA134033@roeck-us.net
> 
> As David Laight suggested, fix this by change MSG_CMSG_COMPAT to
> some value else. MSG_CMSG_COMPAT is an internal value, which is't
> used in userspace, so this change works.
> 

Maybe I am overly concerned (or maybe call it pessimistic),
but I do wonder if this change is worth the added risk. Personally
I'd rather start changing all 'int flag' uses to 'unsigned int flag'
and, only after that is complete, make the switch to BIT().

Of course, that is just my personal opinion, and, as I said,
I may be overly concerned.

Guenter

> Reported-by: Guenter Roeck <linux@...ck-us.net>
> Signed-off-by: Menglong Dong <dong.menglong@....com.cn>
> ---
> v2:
> - add comment to stop people from trying to use BIT(31)
> ---
>  include/linux/socket.h | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/socket.h b/include/linux/socket.h
> index d5ebfe30d96b..61b2694d68dd 100644
> --- a/include/linux/socket.h
> +++ b/include/linux/socket.h
> @@ -312,17 +312,21 @@ struct ucred {
>  					 * plain text and require encryption
>  					 */
>  
> +#if defined(CONFIG_COMPAT)
> +#define MSG_CMSG_COMPAT		BIT(21)	/* This message needs 32 bit fixups */
> +#else
> +#define MSG_CMSG_COMPAT		0	/* We never have 32 bit fixups */
> +#endif
> +
>  #define MSG_ZEROCOPY		BIT(26)	/* Use user data in kernel path */
>  #define MSG_FASTOPEN		BIT(29)	/* Send data in TCP SYN */
>  #define MSG_CMSG_CLOEXEC	BIT(30)	/* Set close_on_exec for file
>  					 * descriptor received through
>  					 * SCM_RIGHTS
>  					 */
> -#if defined(CONFIG_COMPAT)
> -#define MSG_CMSG_COMPAT		BIT(31)	/* This message needs 32 bit fixups */
> -#else
> -#define MSG_CMSG_COMPAT		0	/* We never have 32 bit fixups */
> -#endif
> +/* Attention: Don't use BIT(31) for MSG_*, as 'msg_flags' is defined
> + * as 'int' somewhere and BIT(31) will make it become negative.
> + */
>  
>  
>  /* Setsockoptions(2) level. Thanks to BSD these must match IPPROTO_xxx */
>