Re: [bug report] tcp: Convert tcp-md5 to use MD5 library instead of crypto_ahash

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Oct 22, 2025 at 01:56:47PM +0300, Dan Carpenter wrote:
> Hello Eric Biggers,
> 
> Commit 37a183d3b7cd ("tcp: Convert tcp-md5 to use MD5 library instead
> of crypto_ahash") from Oct 14, 2025 (linux-next), leads to the
> following Smatch static checker warning:
> 
> 	net/ipv4/tcp.c:4911 tcp_inbound_md5_hash()
> 	error: we previously assumed 'key' could be null (see line 4900)
> 
> net/ipv4/tcp.c
>   4884  tcp_inbound_md5_hash(const struct sock *sk, const struct sk_buff *skb,
>   4885                       const void *saddr, const void *daddr,
>   4886                       int family, int l3index, const __u8 *hash_location)
>   4887  {
>   4888          /* This gets called for each TCP segment that has TCP-MD5 option.
>   4889           * We have 3 drop cases:
>   4890           * o No MD5 hash and one expected.
>   4891           * o MD5 hash and we're not expecting one.
>   4892           * o MD5 hash and its wrong.
>   4893           */
>   4894          const struct tcp_sock *tp = tcp_sk(sk);
>   4895          struct tcp_md5sig_key *key;
>   4896          u8 newhash[16];
>   4897  
>   4898          key = tcp_md5_do_lookup(sk, l3index, saddr, family);
>   4899  
>   4900          if (!key && hash_location) {
> 
> If key is NULL and hash_location is zero
> 
>   4901                  NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5UNEXPECTED);
>   4902                  trace_tcp_hash_md5_unexpected(sk, skb);
>   4903                  return SKB_DROP_REASON_TCP_MD5UNEXPECTED;
>   4904          }
>   4905  
>   4906          /* Check the signature.
>   4907           * To support dual stack listeners, we need to handle
>   4908           * IPv4-mapped case.
>   4909           */
>   4910          if (family == AF_INET)
>   4911                  tcp_v4_md5_hash_skb(newhash, key, NULL, skb);
>   4912          else
>   4913                  tp->af_specific->calc_md5_hash(newhash, key, NULL, skb);
> 
> then we are toasted one way or the other.
> 
>   4914          if (memcmp(hash_location, newhash, 16) != 0) {
>   4915                  NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPMD5FAILURE);
>   4916                  trace_tcp_hash_md5_mismatch(sk, skb);
>   4917                  return SKB_DROP_REASON_TCP_MD5FAILURE;
>   4918          }
>   4919          return SKB_NOT_DROPPED_YET;
>   4920  }

Thanks.  I don't think there's a problem with this patch: it just
simplified the code, which happened to make this warning visible.  If
both key and hash_location are NULL, then 'key' gets dereferenced in
tcp_md5_hash_key(), both before and after this patch.  If only
'hash_location' is NULL, then it gets dereferenced when comparing the
hash values.  Before this patch it was conditional on
tcp_v4_md5_hash_skb() succeeding, whereas after it's unconditional.  But
tcp_v4_md5_hash_skb() should never have failed anyway, and even if it
did, its failure or success was unrelated to hash_location.

Looking at the calling code in tcp_inbound_hash(), it actually
guarantees hash_location != NULL.  So, that's why it works.

So, the misleading null check of hash_location in tcp_inbound_md5_hash()
should just be deleted.

- Eric