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
signature.asc
Description: PGP signature
