if_ovpn: free crp, mbuf, and release refcount on crypto_dispatch() failure

[ing.castellanosdz_gmail.com]

Summary

When crypto_dispatch() or crypto_dispatch_async() returns non-zero,
the registered callback is never invoked. In both ovpn_transmit_to_peer()
and ovpn_udp_input(), if_ovpn.c did not free the cryptop request, release
the peer/sc reference count, or free the mbuf on dispatch failure.

This results in three simultaneous leaks per failed dispatch:

• crp allocated via crypto_getreq() is never freed
• peer->refcount (encrypt) or sc->refcount (decrypt) incremented but never decremented
• mbuf passed to crypto_use_mbuf() is never freed

The leaks are reachable under memory pressure when the OCF scheduler
returns ENOMEM from crypto_dispatch(). The registered callbacks
(ovpn_encrypt_tx_cb, ovpn_decrypt_rx_cb) correctly handle crp_etype
for crypto operation failures; this fix addresses the separate
dispatch-level failure path where no callback is invoked.

Found during code review following FreeBSD-SA-26:52.if_wg.

PR:

Test Plan

Code inspection. Verified that crypto_dispatch() failure path does not
invoke the registered callback, confirming crp, mbuf, and refcount are
orphaned. No functional change in the success path.

[kp]

.arcconfig

1 ↗

(On Diff #183773)

You probably didn't mean for this to be part of the commit.

sys/net/if_ovpn.c

2196

That seems pointless, given that it's the next line after this 'if' block.

[kp]

sys/net/if_ovpn.c

2647

I think this is wrong.

The ovpn_udp_input() return value tells the udp stack whether we've grabbed the packet or not. If we return 'false' it's going to try to process the mbuf, which we've just freed.

We should still return true here, indicating that we own the mbuf now.

[ing.castellanosdz_gmail.com]

Address reviewer feedback from kp

• Remove .arcconfig change (not part of this fix)
• Remove redundant return (ret) in encrypt path
• Change return (false) to return (true) in decrypt path: ovpn_udp_input() return value signals mbuf ownership to UDP stack; must return true even on dispatch failure since we free the mbuf

[ing.castellanosdz_gmail.com]

Thank you for the review, kp.

Updated in Diff 183778:

• .arcconfig: reverted to repository original, excluded from this change
• ovpn_transmit_to_peer(): removed redundant return (ret) inside the error block
• ovpn_udp_input(): changed return (false) to return (true); understood that the return value signals mbuf ownership to the UDP stack, and returning false would cause a use-after-free on the mbuf we just freed

[kp]

Thanks for the fix.