netdev - [RFC] Questionable RCU/BH usage in cgw_create

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-next>] [day] [month] [year] [list]

Date: Tue, 31 Oct 2023 12:23:49 +0100
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Oliver Hartkopp <socketcan@...tkopp.net>,
	Marc Kleine-Budde <mkl@...gutronix.de>
Cc: linux-can@...r.kernel.org, netdev@...r.kernel.org,
	Thomas Gleixner <tglx@...utronix.de>
Subject: [RFC] Questionable RCU/BH usage in cgw_create_job().

Hi,

I stumbled over this piece in cgw_create_job():
|      /* update modifications with disabled softirq & quit */
|      local_bh_disable();
|      memcpy(&gwj->mod, &mod, sizeof(mod));
|      local_bh_enable();
|      return 0;

unfortunately the comment did not provide much enlightenment for me.
Let's look. That memcpy() overwrites struct cf_mod within struct cgw_job
which is under RCU protection. memcpy() and RCU hardly works as a combo.
But why the local_bh_disable()?

Let's look further. The user of this data structure is can_can_gw_rcv().
There is something like:
|                 /* check for checksum updates */
|                 if (gwj->mod.csumfunc.crc8)
|                         (*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);

With optimisation enabled (as in -O2 or so) the compiler will fetch
mod.csumfunc.crc8, do the comparison and if non-NULL use the previously
fetched value and jump there. So one could argue that it is not really
affected by the memcpy() suddenly setting it to NULL. However, adding
any kind of a function in between, say 
|                 /* check for checksum updates */
|                 if (gwj->mod.csumfunc.crc8) {
|+                        trace_event_crc8_sth(cf)
|                         (*gwj->mod.csumfunc.crc8)(cf, &gwj->mod.csum.crc8);
|                 }

will force the compiler to reload mod.csumfunc.crc8. And here is the
possible NULL pointer if overwritten by update in cgw_create_job().

One reload that already happens is the one of mod.modfunc. First at the
top we have:
|         if (gwj->mod.modfunc[0])
|                 nskb = skb_copy(skb, GFP_ATOMIC);
|         else
|                 nskb = skb_clone(skb, GFP_ATOMIC);

Here mod.modfunc[0] is NULL and skb_clone() is invoked. Later if
mod.modfunc has been set to non-NULL value this piece
|         while (modidx < MAX_MODFUNCTIONS && gwj->mod.modfunc[modidx])
|                 (*gwj->mod.modfunc[modidx++])(cf, &gwj->mod);

reloads mod.modfunc and may modify the skb assuming that skb_copy() has
been used earlier.

Looking at this makes me think that the local_bh_disable() has been
added to the memcpy() just to ensure that can_can_gw_rcv() won't run
because it is invoked in a BH disabled context. Clever little trick that
is. But this trick is limited to UP environments…

Am I missing something?

If not, my suggestion would be replacing the bh-off, memcpy part with:
|		old_mod = rcu_replace_pointer(gwj->mod, new_mod, true);
|		kfree_rcu_mightsleep(old_mod);

and doing the needed pointer replacement with for struct cgw_job::mod
and RCU annotation.

Sebastian