[bug report] ovpn: implement packet processing

Hello Antonio Quartulli,

Commit 8534731dbf2d ("ovpn: implement packet processing") from Apr
15, 2025 (linux-next), leads to the following Smatch static checker
warning:

drivers/net/ovpn/io.c:207 ovpn_decrypt_post() warn: variable dereferenced before check 'peer' (see line 148)
drivers/net/ovpn/io.c:305 ovpn_encrypt_post() warn: variable dereferenced before check 'peer' (see line 304)

drivers/net/ovpn/io.c
    108 void ovpn_decrypt_post(void *data, int ret)
    109 {
    110         struct ovpn_crypto_key_slot *ks;
    111         unsigned int payload_offset = 0;
    112         struct sk_buff *skb = data;
    113         struct ovpn_socket *sock;
    114         struct ovpn_peer *peer;
    115         __be16 proto;
    116         __be32 *pid;
    117 
    118         /* crypto is happening asynchronously. this function will be called
    119          * again later by the crypto callback with a proper return code
    120          */
    121         if (unlikely(ret == -EINPROGRESS))
    122                 return;
    123 
    124         payload_offset = ovpn_skb_cb(skb)->payload_offset;
    125         ks = ovpn_skb_cb(skb)->ks;
    126         peer = ovpn_skb_cb(skb)->peer;
    127 
    128         /* crypto is done, cleanup skb CB and its members */
    129         kfree(ovpn_skb_cb(skb)->crypto_tmp);
    130 
    131         if (unlikely(ret < 0))
    132                 goto drop;
    133 
    134         /* PID sits after the op */
    135         pid = (__force __be32 *)(skb->data + OVPN_OPCODE_SIZE);
    136         ret = ovpn_pktid_recv(&ks->pid_recv, ntohl(*pid), 0);
    137         if (unlikely(ret < 0)) {
    138                 net_err_ratelimited("%s: PKT ID RX error for peer %u: %d\n",
    139                                     netdev_name(peer->ovpn->dev), peer->id,
    140                                     ret);
    141                 goto drop;
    142         }
    143 
    144         /* keep track of last received authenticated packet for keepalive */
    145         WRITE_ONCE(peer->last_recv, ktime_get_real_seconds());
                           ^^^^^^^^^^^^^^^
Most of the function assumes peer can't be NULL

    146 
    147         rcu_read_lock();
    148         sock = rcu_dereference(peer->sock);
    149         if (sock && sock->sk->sk_protocol == IPPROTO_UDP)
    150                 /* check if this peer changed local or remote endpoint */
    151                 ovpn_peer_endpoints_update(peer, skb);
    152         rcu_read_unlock();
    153 
    154         /* point to encapsulated IP packet */
    155         __skb_pull(skb, payload_offset);
    156 
    157         /* check if this is a valid datapacket that has to be delivered to the
    158          * ovpn interface
    159          */
    160         skb_reset_network_header(skb);
    161         proto = ovpn_ip_check_protocol(skb);
    162         if (unlikely(!proto)) {
    163                 /* check if null packet */
    164                 if (unlikely(!pskb_may_pull(skb, 1))) {
    165                         net_info_ratelimited("%s: NULL packet received from peer %u\n",
    166                                              netdev_name(peer->ovpn->dev),
    167                                              peer->id);
    168                         goto drop;
    169                 }
    170 
    171                 if (ovpn_is_keepalive(skb)) {
    172                         net_dbg_ratelimited("%s: ping received from peer %u\n",
    173                                             netdev_name(peer->ovpn->dev),
    174                                             peer->id);
    175                         /* we drop the packet, but this is not a failure */
    176                         consume_skb(skb);
    177                         goto drop_nocount;
    178                 }
    179 
    180                 net_info_ratelimited("%s: unsupported protocol received from peer %u\n",
    181                                      netdev_name(peer->ovpn->dev), peer->id);
    182                 goto drop;
    183         }
    184         skb->protocol = proto;
    185 
    186         /* perform Reverse Path Filtering (RPF) */
    187         if (unlikely(!ovpn_peer_check_by_src(peer->ovpn, skb, peer))) {
    188                 if (skb->protocol == htons(ETH_P_IPV6))
    189                         net_dbg_ratelimited("%s: RPF dropped packet from peer %u, src: %pI6c\n",
    190                                             netdev_name(peer->ovpn->dev),
    191                                             peer->id, &ipv6_hdr(skb)->saddr);
    192                 else
    193                         net_dbg_ratelimited("%s: RPF dropped packet from peer %u, src: %pI4\n",
    194                                             netdev_name(peer->ovpn->dev),
    195                                             peer->id, &ip_hdr(skb)->saddr);
    196                 goto drop;
    197         }
    198 
    199         ovpn_netdev_write(peer, skb);
    200         /* skb is passed to upper layer - don't free it */
    201         skb = NULL;
    202 drop:
    203         if (unlikely(skb))
    204                 dev_dstats_rx_dropped(peer->ovpn->dev);
    205         kfree_skb(skb);
    206 drop_nocount:
--> 207         if (likely(peer))
                           ^^^^
So hopefully this NULL check could be removed as well?

    208                 ovpn_peer_put(peer);
    209         if (likely(ks))

Same for ks and also in the encrypt function.

    210                 ovpn_crypto_key_slot_put(ks);
    211 }

This email is a free service from the Smatch-CI project [smatch.sf.net].

regards,
dan carpenter