netdev - Re: [PATCH net-next 2/3] net-timestamp: allow reading recv cmsg on errqueue with origin tstamp

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+FuTSewRhdTk5ymVowd8xFUNtQDOUnOMjhXMPmQccKtatprrQ@mail.gmail.com>
Date:	Mon, 1 Dec 2014 10:59:43 -0500
From:	Willem de Bruijn <willemb@...gle.com>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Network Development <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH net-next 2/3] net-timestamp: allow reading recv cmsg on
 errqueue with origin tstamp

>>>> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
>>>> index 59eba6c..640f26c 100644
>>>> --- a/net/ipv4/ip_sockglue.c
>>>> +++ b/net/ipv4/ip_sockglue.c
>>>> @@ -399,6 +399,22 @@ void ip_local_error(struct sock *sk, int err, __be32 daddr, __be16 port, u32 inf
>>>>                 kfree_skb(skb);
>>>>  }
>>>>
>>>> +static bool ipv4_pktinfo_prepare_errqueue(const struct sock *sk,
>>>> +                                         const struct sk_buff *skb,
>>>> +                                         int ee_origin)
>>>> +{
>>>> +       struct in_pktinfo *info = PKTINFO_SKB_CB(skb);
>>>> +
>>>> +       if ((ee_origin != SO_EE_ORIGIN_TIMESTAMPING) ||
>>>> +           (!(sk->sk_tsflags & SOF_TIMESTAMPING_OPT_CMSG)) ||
>>>> +           (!skb->dev))
>>>> +               return false;
>>
>> This function is called to decide whether to call ip_cmsg_recv when
>> the origin is not SO_EE_ORIGIN_ICMP. For other origins, the pktinfo
>> field is not initialized, so we initialize it here.
>>
>> The socket owner must still set socket option IP_PKTINFO to exercise
>> the relevant code in ip_cmsg_recv:
>>
>>         unsigned int flags = inet->cmsg_flags;
>>
>>         if (flags & 1)
>>                 ip_cmsg_recv_pktinfo(msg, skb);
>>
>> and
>>
>> net/ipv4/ip_sockglue.c:48:#define IP_CMSG_PKTINFO 1
>>
>
> Aha, got it.  I'm really not very familiar with the network plumbing
> -- I just use it from the userspace side and occasionally try to write
> patches when something doesn't work :)
>
>>
>>>> +
>>>> +       info->ipi_spec_dst.s_addr = ip_hdr(skb)->saddr;
>>>
>>> Is this the source addr chosen by the initial routing decision when
>>> the packet was sent, or is it the final source addr on the way out?
>>> If the latter, is this an information leak when network namespaces are
>>> in use?
>>
>> Not as long as the entire payload is looped back with the metadata,
>> as it is currently. Note my suggested fix at the top: to give processes
>> an option to request timestamps without either, and to give the
>> administrator the option to drop all others.
>
> So what happens to ipi_spec_dst if that admin option is set?

I would drop the packet completely. Drop with payload has to be
implemented as a separate check before skb_copy_datagram_msg
anyway.

It is possible to return the timestamp, but zero the fields, but I
find that harder to reason about, so it may cause subtle
process bugs.

A related question is what this field holds if the process
requests cmsg, but no payload. Again, this is probably
best treated as an illegal combination of options and
should fail hard.

>
> --Andy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html