Message-ID: <20180926094922.y3gg2kzui4j6fvdx@brauner.io>
Date:   Wed, 26 Sep 2018 11:49:23 +0200
From:   Christian Brauner <christian@...uner.io>
To:     David Ahern <dsahern@...il.com>
Cc:     Jiri Benc <jbenc@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>
Subject: Re: netlink: 16 bytes leftover after parsing attributes in process
 `ip'.

On Tue, Sep 25, 2018 at 09:37:41AM -0600, David Ahern wrote:
> On 9/25/18 8:47 AM, Jiri Benc wrote:
> > On Tue, 25 Sep 2018 11:49:10 +0200, Christian Brauner wrote:
> >> So if people really want to hide this issue as much as we can then we
> >> can play the guessing game. I could send a patch that roughly does the
> >> following:
> >>
> >> if (nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg))
> >>         guessed_header_len = sizeof(struct ifaddrmsg);
> >> else
> >>         guessed_header_len = sizeof(struct ifinfomsg);
> >>
> >> This will work since sizeof(ifaddrmsg) == 8 and sizeof(ifinfomsg) == 16.
> >> The only valid property for RTM_GETADDR requests is IFA_TARGET_NETNSID.
> >> This propert is a __s32 which should bring the message up to 12 bytes
> >> (not sure about alignment requiremnts and where we might wend up ten)
> >> which is still less than the 16 bytes without that property from
> >> ifinfomsg. That's a hacky hacky hack-hack and will likely work but will
> >> break when ifaddrmsg grows a new member or we introduce another property
> >> that is valid in RTM_GETADDR requests. It also will not work cleanly
> >> when users stuff additional properties in there that are vaif (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,lid for the
> >> address family but are not used int RTM_GETADDR requests.
> > 
> > I'd expect that any potential existing code that makes use of other
> > attributes already assumes ifaddrmsg. Hence, if the nlmsg_len is >
> > sizeof(ifinfomsg), you can be sure that there are attributes and thus
> > the struct used was ifaddrmsg.
> > 
> > So, in order for RTM_GETADDR to work reliably with attributes, you have
> > to ensure that the length is > sizeof(ifinfomsg).
> 
> One of the many on-going efforts I have in progress is kernel side
> filtering of route dumps. It has this same problem. For it I am
> proposing a new flag:
> 
> #define RTM_F_PROPER_HEADER    0x4000
> 
> ifinfomsg is 16 bytes which is > rtmsg at 12. If the message length is >
> 16, then rtm_flags can be checked to know if the proper header is sent.
> 
> For ifaddrmsg things do not line up as well. Worst all of the flag bits
> are used. But, perhaps one can be overloaded with the limit that you can
> never filter on its presence. Since you are adding the first filter to
> address dumps such a limitation should be ok.
> 
> For example something like this (whitespace damaged on paste) to remove
> the guess work:
> 
> diff --git a/include/uapi/linux/if_addr.h b/include/uapi/linux/if_addr.h
> index dfcf3ce0097f..8e3e9d475db5 100644
> --- a/include/uapi/linux/if_addr.h
> +++ b/include/uapi/linux/if_addr.h
> @@ -41,6 +41,11 @@ enum {
>  #define IFA_MAX (__IFA_MAX - 1)
> 
>  /* ifa_flags */
> +/* IFA_F_PROPER_HEADER is only set in ifa_flags for dumps
> + * to indicate the ancillary header is the expected ifaddrmsg
> + * vs ifinfomsg from legacy userspace
> + */
> +#define IFA_F_PROPER_HEADER    0x01
>  #define IFA_F_SECONDARY                0x01
>  #define IFA_F_TEMPORARY                IFA_F_SECONDARY
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index bfe3ec7ecb14..256b9f88db8f 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -5022,8 +5022,13 @@ static int inet6_dump_addr(struct sk_buff *skb,
> struct netlink_callback *cb,
>         s_idx = idx = cb->args[1];
>         s_ip_idx = ip_idx = cb->args[2];
> 
> -       if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX,
> -                       ifa_ipv6_policy, NULL) >= 0) {
> +       if (nlmsg_len(cb->nlh) >= sizeof(struct ifaddrmsg) &&
> +           ((struct ifaddrmsg *) nlmsg_data(cb->nlh))->ifa_flags &
> IFA_F_PROPER_HEADER) {
> +
> +               if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb,
> IFA_MAX,
> +                               ifa_ipv6_policy, NULL) >= 0) {
> +       ...
> +
>                 if (tb[IFA_TARGET_NETNSID]) {
>                         netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]);
> 
> 
> For ifaddrmsg ifa_flags aligns with ifi_type which is set by kernel side
> so this should be ok.

I like this idea way more than forcing userspace to create a nested
attribute. If Jiri's question is answered can we get this patch on the
road soon?
Are you happy to send it on-top of net-next or do you want me to do it?

Christian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists