On Sat, Mar 19, 2022 at 12:27:43PM +0100, Claudio Jeker wrote:
> 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?
Actually in_pcbselsrc() checks the multicast options but only for the
IN_MULTICAST() case. I guess we could add INADDR_BROADCAST handling in
there as well. Seems like a sensible thing todo. Broadcast is just a very
special version of multicast.
Also if this is changed is there still a way to have ip->ip_src ==
INADDR_ANY in ip_output()?
> > > 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
>
--
:wq Claudio
