netdev - Re: [Race] data race between eth_heder_cache_update() and neigh_hh

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: <20201130155342.GK2730@breakpoint.cc>
Date:   Mon, 30 Nov 2020 16:53:42 +0100
From:   Florian Westphal <fw@...len.de>
To:     "Gong, Sishuai" <sishuai@...due.edu>
Cc:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [Race] data race between eth_heder_cache_update() and
 neigh_hh_output()

Gong, Sishuai <sishuai@...due.edu> wrote:
> Hi,
> 
> We found a data race in linux kernel 5.3.11 that we are able to reproduce in x86 under specific interleavings. We are not sure about the consequence of this race now but it seems that the two memcpy() can lead to some inconsistency. We also noticed that both the writer and reader are protected by locks, but the writer is protected using seqlock while the reader is protected by rculock.

AFAICS reader uses same seqlock as the writer.

> ------------------------------------------
> Write site
> 
>  /tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/net/ethernet/eth.c:264
>         252  /**
>         253   * eth_header_cache_update - update cache entry
>         254   * @hh: destination cache entry
>         255   * @dev: network device
>         256   * @haddr: new hardware address
>         257   *
>         258   * Called by Address Resolution module to notify changes in address.
>         259   */
>         260  void eth_header_cache_update(struct hh_cache *hh,
>         261                   const struct net_device *dev,
>         262                   const unsigned char *haddr)
>         263  {
>  ==>    264      memcpy(((u8 *) hh->hh_data) + HH_DATA_OFF(sizeof(struct ethhdr)),
>         265             haddr, ETH_ALEN);

This is called under write_seqlock_bh(hh->hh_lock)

> /tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/include/net/neighbour.h:481
>         463  static inline int neigh_hh_output(const struct hh_cache *hh, struct sk_buff *skb)
>         464  {
>         465      unsigned int hh_alen = 0;
>         466      unsigned int seq;
>         467      unsigned int hh_len;
>         468
>         469      do {
>         470          seq = read_seqbegin(&hh->hh_lock); 

This samples the seqcount.

>         471          hh_len = hh->hh_len;
>         472          if (likely(hh_len <= HH_DATA_MOD)) {
>         473              hh_alen = HH_DATA_MOD;
>         474
>         475              /* skb_push() would proceed silently if we have room for
>         476               * the unaligned size but not for the aligned size:
>         477               * check headroom explicitly.
>         478               */
>         479              if (likely(skb_headroom(skb) >= HH_DATA_MOD)) {
>         480                  /* this is inlined by gcc */
>  ==>    481                  memcpy(skb->data - HH_DATA_MOD, hh->hh_data,
>         482                         HH_DATA_MOD);

[..]

>         492      } while (read_seqretry(&hh->hh_lock, seq));

... so this retries unless seqcount was stable.