On Sat, Mar 19, 2022 at 10:51:13AM +0000, Stuart Henderson wrote:
> On 2022/03/19 19:14, David Gwynne wrote:
> > On Thu, Mar 17, 2022 at 12:05:16PM -0600, Theo de Raadt wrote:
> > > This should not be done in applications. The kernel must do it. It means
> > > the current kernel code is worng.
> >
> > i think this is the right place for raw ipv4 sockets like what
> > ping/traceroute uses.
> >
> > ipv6 looks like it already does the right thing.
>
> Given that this turned out so similar to the existing v6 support that
> I think you didn't notice before writing the v4 version, this looks
> like the right place indeed :)
>
> Works for me, OK
Did someone try this on an ospf router?
I think our ospfd(8) always includes the source address and uses IP_HDRINCL
but not sure about other daemons. Are there other raw IP users that expect
the IP source to be set to the outgoing interface by default?
Maybe IGMP proxies and routers. E.g. dvmrpd depends on IP_MULTICAST_IF to
set the source IP address.
Looking at the code I think this will break the IP_MULTICAST_IF case.
In ip_output() the check is like this:
if ((IN_MULTICAST(ip->ip_dst.s_addr) ||
(ip->ip_dst.s_addr == INADDR_BROADCAST)) &&
imo != NULL && (ifp = if_get(imo->imo_ifidx)) != NULL) {
mtu = ifp->if_mtu;
if (ip->ip_src.s_addr == INADDR_ANY) {
struct in_ifaddr *ia;
IFP_TO_IA(ifp, ia);
if (ia != NULL)
ip->ip_src = ia->ia_addr.sin_addr;
}
Now raw_ip.c already set ip_src to something so
if (ip->ip_src.s_addr == INADDR_ANY)
is never true.
It is possible to skip the in_pcbselsrc() call in rip_output() when
IN_MULTICAST(ip->ip_dst.s_addr) || (ip->ip_dst.s_addr == INADDR_BROADCAST)
Is that good enough?
> > Index: raw_ip.c
> > ===================================================================
> > RCS file: /cvs/src/sys/netinet/raw_ip.c,v
> > retrieving revision 1.123
> > diff -u -p -r1.123 raw_ip.c
> > --- raw_ip.c 14 Mar 2022 22:38:43 -0000 1.123
> > +++ raw_ip.c 19 Mar 2022 03:40:44 -0000
> > @@ -222,6 +222,7 @@ int
> > rip_output(struct mbuf *m, struct socket *so, struct sockaddr *dstaddr,
> > struct mbuf *control)
> > {
> > + struct sockaddr_in *dst = satosin(dstaddr);
> > struct ip *ip;
> > struct inpcb *inp;
> > int flags, error;
> > @@ -246,8 +247,8 @@ rip_output(struct mbuf *m, struct socket
> > ip->ip_off = htons(0);
> > ip->ip_p = inp->inp_ip.ip_p;
> > ip->ip_len = htons(m->m_pkthdr.len);
> > - ip->ip_src = inp->inp_laddr;
> > - ip->ip_dst = satosin(dstaddr)->sin_addr;
> > + ip->ip_src.s_addr = INADDR_ANY;
> > + ip->ip_dst = dst->sin_addr;
> > ip->ip_ttl = inp->inp_ip.ip_ttl ? inp->inp_ip.ip_ttl : MAXTTL;
> > } else {
> > if (m->m_pkthdr.len > IP_MAXPACKET) {
> > @@ -262,11 +263,23 @@ rip_output(struct mbuf *m, struct socket
> > ip = mtod(m, struct ip *);
> > if (ip->ip_id == 0)
> > ip->ip_id = htons(ip_randomid());
> > + dst->sin_addr = ip->ip_dst;
> >
> > /* XXX prevent ip_output from overwriting header fields */
> > flags |= IP_RAWOUTPUT;
> > ipstat_inc(ips_rawout);
> > }
> > +
> > + if (ip->ip_src.s_addr == INADDR_ANY) {
> > + struct in_addr *laddr;
> > +
> > + error = in_pcbselsrc(&laddr, dst, inp);
> > + if (error != 0)
> > + return (error);
> > +
> > + ip->ip_src = *laddr;
> > + }
> > +
> > #ifdef INET6
> > /*
> > * A thought: Even though raw IP shouldn't be able to set IPv6
> >
>
--
:wq Claudio
