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

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

[Dan Carpenter]

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  }

regards,
dan carpenter

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

[Eric Biggers]

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