ok mvs@

> On 13 May 2021, at 02:43, YASUOKA Masahiko <yasu...@openbsd.org> wrote:
> 
> On Wed, 12 May 2021 19:15:29 +0300
> Vitaliy Makkoveev <m...@openbsd.org> wrote:
>>> On 12 May 2021, at 18:42, YASUOKA Masahiko <yasu...@openbsd.org> wrote:
>>> On Wed, 12 May 2021 17:26:51 +0300
>>> Vitaliy Makkoveev <m...@openbsd.org> wrote:
>>>> On Wed, May 12, 2021 at 07:11:09PM +0900, YASUOKA Masahiko wrote:
>>>>> Radek reported a problem to misc@ that multiple Windows clients behind a 
>>>>> NAT
>>>>> cannot use a L2TP/IPsec server simultaneously.
>>>>> 
>>>>> https://marc.info/?t=160996816100001&r=1&w=2
>>>>> 
>>>>> There is two problems.  First is pipex(4) doesn't pass the proper
>>>>> ipsecflowinfo to ip_output().  Second is the IPsec policy check which is
>>>>> done by ipsp_spd_lookup() returns -1 (EINVAL) if the given tdb is not
>>>>> cached.  This happens when its flow is shared by another tdb (for another
>>>>> client of the same NAT).
>>>>> 
>>>>> The following 2 diffs fix these problem.
>>>>> 
>>>>> comment?
>>>>> ok?
>>>>> 
>>>> 
>>>> Hi.
>>>> 
>>>> I have two comments for the diff 1:
>>>> 
>>>> 1. You should add PACKET_TAG_IPSEC_FLOWINFO description to
>>>>   m_tag_get(9).
>>>> 2. You introduced mbuf(9) leak in pipex_l2tp_output() error path. I
>>>>  pointed the place in your diff.
>>> 
>>> Good catch.  Thanks.
>>> 
>> 
>> m_freem(9) accepts NULL so this check before is redundant.
> 
> Yes,
> 
>> It seems to me that "Used by the IPv4 stack to specify the IPsec flow
>> of an output IP packet. The tag contains a u_int32_t identifying the
>> IPsec flow.” is enough. Anyway it’s better to ask jmc@.
> 
> Ok,
> 
>> Also I like to remove PACKET_TAG_PIPEX with separate diff.
> 
> I removed PACKET_TAG_PIPEX separetely.  
> 
> Let me update the diff.
> 
> Index: sys/net/pipex.c
> ===================================================================
> RCS file: /disk/cvs/openbsd/src/sys/net/pipex.c,v
> retrieving revision 1.132
> diff -u -p -r1.132 pipex.c
> --- sys/net/pipex.c   10 Mar 2021 10:21:48 -0000      1.132
> +++ sys/net/pipex.c   12 May 2021 23:18:52 -0000
> @@ -1628,6 +1628,7 @@ pipex_l2tp_output(struct mbuf *m0, struc
> #ifdef INET6
>       struct ip6_hdr *ip6;
> #endif
> +     struct m_tag *mtag;
> 
>       hlen = sizeof(struct pipex_l2tp_header) +
>           ((pipex_session_is_l2tp_data_sequencing_on(session))
> @@ -1704,6 +1705,15 @@ pipex_l2tp_output(struct mbuf *m0, struc
>               ip->ip_tos = 0;
>               ip->ip_off = 0;
> 
> +             if (session->proto.l2tp.ipsecflowinfo > 0) {
> +                     if ((mtag = m_tag_get(PACKET_TAG_IPSEC_FLOWINFO,
> +                         sizeof(u_int32_t), M_NOWAIT)) == NULL)
> +                             goto drop;
> +                     *(u_int32_t *)(mtag + 1) =
> +                         session->proto.l2tp.ipsecflowinfo;
> +                     m_tag_prepend(m0, mtag);
> +             }
> +
>               ip_send(m0);
>               break;
> #ifdef INET6
> @@ -1733,6 +1743,7 @@ pipex_l2tp_output(struct mbuf *m0, struc
> 
>       return;
> drop:
> +     m_freem(m0);
>       session->stat.oerrors++;
> }
> 
> Index: sys/netinet/ip_input.c
> ===================================================================
> RCS file: /disk/cvs/openbsd/src/sys/netinet/ip_input.c,v
> retrieving revision 1.359
> diff -u -p -r1.359 ip_input.c
> --- sys/netinet/ip_input.c    30 Apr 2021 13:52:48 -0000      1.359
> +++ sys/netinet/ip_input.c    12 May 2021 23:18:52 -0000
> @@ -1790,6 +1790,8 @@ ip_send_do_dispatch(void *xmq, int flags
>       struct mbuf_queue *mq = xmq;
>       struct mbuf *m;
>       struct mbuf_list ml;
> +     struct m_tag *mtag;
> +     u_int32_t ipsecflowinfo = 0;
> 
>       mq_delist(mq, &ml);
>       if (ml_empty(&ml))
> @@ -1797,7 +1799,12 @@ ip_send_do_dispatch(void *xmq, int flags
> 
>       NET_LOCK();
>       while ((m = ml_dequeue(&ml)) != NULL) {
> -             ip_output(m, NULL, NULL, flags, NULL, NULL, 0);
> +             if ((mtag = m_tag_find(m, PACKET_TAG_IPSEC_FLOWINFO, NULL))
> +                 != NULL) {
> +                     ipsecflowinfo = *(u_int32_t *)(mtag + 1);
> +                     m_tag_delete(m, mtag);
> +             }
> +             ip_output(m, NULL, NULL, flags, NULL, NULL, ipsecflowinfo);
>       }
>       NET_UNLOCK();
> }
> Index: sys/sys/mbuf.h
> ===================================================================
> RCS file: /disk/cvs/openbsd/src/sys/sys/mbuf.h,v
> retrieving revision 1.252
> diff -u -p -r1.252 mbuf.h
> --- sys/sys/mbuf.h    25 Feb 2021 02:43:31 -0000      1.252
> +++ sys/sys/mbuf.h    12 May 2021 23:18:52 -0000
> @@ -469,6 +469,7 @@ struct m_tag *m_tag_next(struct mbuf *, 
> /* Packet tag types */
> #define PACKET_TAG_IPSEC_IN_DONE      0x0001  /* IPsec applied, in */
> #define PACKET_TAG_IPSEC_OUT_DONE     0x0002  /* IPsec applied, out */
> +#define PACKET_TAG_IPSEC_FLOWINFO    0x0004  /* IPsec flowinfo */
> #define PACKET_TAG_WIREGUARD          0x0040  /* WireGuard data */
> #define PACKET_TAG_GRE                        0x0080  /* GRE processing done 
> */
> #define PACKET_TAG_DLT                        0x0100 /* data link layer type 
> */
> @@ -479,7 +480,7 @@ struct m_tag *m_tag_next(struct mbuf *, 
> #define PACKET_TAG_CARP_BAL_IP                0x4000  /* carp(4) ip balanced 
> marker */
> 
> #define MTAG_BITS \
> -    ("\20\1IPSEC_IN_DONE\2IPSEC_OUT_DONE\3IPSEC_IN_CRYPTO_DONE" \
> +    ("\20\1IPSEC_IN_DONE\2IPSEC_OUT_DONE\3IPSEC_FLOWINFO" \
>     "\4IPSEC_OUT_CRYPTO_NEEDED\5IPSEC_PENDING_TDB\6BRIDGE\7WG\10GRE\11DLT" \
>     "\12PF_DIVERT\14PF_REASSEMBLED\15SRCROUTE\16TUNNEL\17CARP_BAL_IP")
> 
> Index: share/man/man9/mbuf_tags.9
> ===================================================================
> RCS file: /disk/cvs/openbsd/src/share/man/man9/mbuf_tags.9,v
> retrieving revision 1.41
> diff -u -p -r1.41 mbuf_tags.9
> --- share/man/man9/mbuf_tags.9        21 Jun 2020 15:25:30 -0000      1.41
> +++ share/man/man9/mbuf_tags.9        12 May 2021 23:18:52 -0000
> @@ -122,6 +122,11 @@ The tag contains a
> identifying the security association applied to the packet.
> This tag is primarily used to detect and avoid loops in IPsec
> processing on output.
> +.It PACKET_TAG_IPSEC_FLOWINFO
> +Used by the IPv4 stack to specify the IPsec flow of an output IP packet.
> +The tag contains a
> +.Va u_int32_t
> +identifying the IPsec flow.
> .It PACKET_TAG_WIREGUARD
> Used by the
> .Xr wg 4

Reply via email to