linux-kernel - Re: [PATCH net-next v24 09/23] ovpn: implement packet processing

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: <CALW65jZ=Jngf0THLTuWHuhpQb0NDM=4x4HN_Xj922nmq71EMUQ@mail.gmail.com>
Date: Wed, 26 Mar 2025 18:03:18 +0800
From: Qingfang Deng <dqfext@...il.com>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, netdev@...r.kernel.org, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Donald Hunter <donald.hunter@...il.com>, Shuah Khan <shuah@...nel.org>, sd@...asysnail.net, 
	ryazanov.s.a@...il.com, Andrew Lunn <andrew+netdev@...n.ch>, 
	Xiao Liang <shaw.leon@...il.com>
Subject: Re: [PATCH net-next v24 09/23] ovpn: implement packet processing

Hi Antonio,

On Wed, Mar 26, 2025 at 5:41 PM Antonio Quartulli <antonio@...nvpn.net> wrote:
> >> +/* Get the next packet ID for xmit */
> >> +static inline int ovpn_pktid_xmit_next(struct ovpn_pktid_xmit *pid, u32 *pktid)
> >> +{
> >> +    const s64 seq_num = atomic64_fetch_add_unless(&pid->seq_num, 1,
> >> +                                                  0x100000000LL);
> >> +    /* when the 32bit space is over, we return an error because the packet
> >> +     * ID is used to create the cipher IV and we do not want to reuse the
> >> +     * same value more than once
> >> +     */
> >> +    if (unlikely(seq_num == 0x100000000LL))
> >> +            return -ERANGE;
> >
> > You may use a 32-bit atomic_t, instead of checking if it equals
> > 0x1_00000000, check if it wraparounds to zero.
> > Additionally, you don't need full memory ordering as you just want an
> > incrementing value:
> >
> > int seq_num = atomic_fetch_inc_relaxed(&pid->seq_num);
> >
> > if (unlikely(!seq_num))
> >       ...
>
> But then if we have concurrent invocations of ovpn_pktid_xmit_next()
> only the first one will error out on wrap-around, while the others will
> return no error (seq_num becomes > 0) and will allow the packets to go
> through.
>
> This is not what we want.

Got it. You could replace it with
atomic_fetch_add_unless(&pid->seq_num, 1, 0) and check if it wraps
around to zero.

However, what about the opposite scenario? If multiple threads
concurrently invoke ovpn_pktid_xmit_next() and all detect the
wraparound condition, could this lead to simultaneous calls to
ovpn_crypto_kill_key() and ovpn_nl_key_swap_notify()?

>
> >
> >> +
> >> +    *pktid = (u32)seq_num;
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +/* Write 12-byte AEAD IV to dest */
> >> +static inline void ovpn_pktid_aead_write(const u32 pktid,
> >> +                                     const u8 nt[],
> >> +                                     unsigned char *dest)
> >> +{
> >> +    *(__force __be32 *)(dest) = htonl(pktid);
> >> +    BUILD_BUG_ON(4 + OVPN_NONCE_TAIL_SIZE != OVPN_NONCE_SIZE);
> >> +    memcpy(dest + 4, nt, OVPN_NONCE_TAIL_SIZE);
> >> +}
> >> +
>
> Qingfang, may I ask you to remove from your reply non-relevant code
> (like I did above), especially in such long patches, as it becomes a bit
> tedious to spot your comments and I may miss something.

Noted.

>
> Thanks a lot!
>
> Regards,
>
> --
> Antonio Quartulli
> OpenVPN Inc.
>