Kasper (https://vusec.net/projects/kasper/) has shown that using the
list_for_each_entry() iterator is security critical with transient
execution in mind.
list_for_each_entry() iterates through a list until the terminating
condition is met where pos, the iterator value, reaches the head element.
The head is usually not within the struct of the list elements and either
stands on its own or is included within a struct of an other type.
Therefore using `container_of` on the head element to retrieve the iterator
value is invalid and therefore a type confusion.
If the terminating condition does not hold in transient execution and is
mispredicted, the iterator infered from the head is used in the additional
transient iteration of the loop body. Depending on the struct members
accessed within the loop an attacker can inject own values turning it into
a transient gadget. In the paper we have showed such a case in
find_keyring_by_name().
We patched list_for_each_entry() to ensure that pos is not pointing to an
invalid head element in the transient iteration of the loop. It uses
branchless logic to set pos to NULL when the terminating condition is met,
making it unusable in transient execution.
Unfortunately there are several locations in the kernel where the list
iterator is used after the loop that break with such a change. Luckily
there is the use_after_iter.cocci script that can be used to identify such
code locations. I had to adapt the script slightly since it reduces false
positives in the original use case but those are relevant for this patch.
A large range of reported code locations only use the list iterator after
the loop if there was an early exit (break/goto) and are therefore not
relevant.
However there is still ~200 occurrences that would break with the nospec
implementation of list_for_each_entry().
I have categorized them into certain categories where some are more trivial
to patch than others. I have included reference patches for the first case
of the categories that are trivial or almost trivial to patch. When
everything is clear I will go through the list to fix the other cases
according to the reference patch.
head pos will reference to the case where pos is not actually pointing to
the element of the list but is derived from the head.
I'd especially appreciate comments on how to deal with category 10.
For those cases it is not clear that head pos is not used incorrectly. It
either relies on implict constraints that ensure that the list iteration
exits early or is an actual bug.
Since non of those cases should use pos if the break was not hit they
should not be affected by the speculative list iterator.
I just want to make sure, since changing the list iterator will turn them
from a type confusion into a null pointer dereference (+ some offset
depending on the struct member).
All line numbers are based on commit f71077a4d84bbe8c7b91b7db7c4ef815755ac5e3.
Category 1: &pos->other_member is used to determine if a certain element was found
drivers/usb/gadget/udc/gr_udc.c:1717
drivers/usb/gadget/udc/net2280.c:1273
drivers/usb/gadget/udc/atmel_usba_udc.c:877
drivers/usb/gadget/udc/lpc32xx_udc.c:1847
drivers/usb/gadget/udc/pxa25x_udc.c:983
drivers/usb/gadget/udc/aspeed-vhub/epn.c:481
drivers/usb/gadget/udc/fsl_qe_udc.c:1793
drivers/usb/gadget/udc/net2272.c:946
drivers/usb/gadget/udc/s3c-hsudc.c:893
drivers/usb/gadget/udc/mv_udc_core.c:800
drivers/usb/gadget/udc/at91_udc.c:724
drivers/usb/gadget/udc/mv_u3d_core.c:868
drivers/usb/gadget/udc/udc-xilinx.c:1149
drivers/usb/gadget/udc/bdc/bdc_ep.c:1779
drivers/usb/gadget/udc/fsl_udc_core.c:947
drivers/usb/gadget/udc/omap_udc.c:1019
Category 2: pos is used to determine if a certain element was found
drivers/vfio/mdev/mdev_core.c:349
drivers/usb/gadget/udc/tegra-xudc.c:1427
drivers/usb/gadget/udc/max3420_udc.c:1062
drivers/thermal/thermal_core.c:1085
drivers/thermal/thermal_core.c:645
drivers/thermal/thermal_core.c:1349
drivers/usb/gadget/configfs.c:435
drivers/usb/gadget/configfs.c:893
drivers/usb/mtu3/mtu3_gadget.c:343
drivers/usb/musb/musb_gadget.c:1285
arch/x86/kernel/cpu/sgx/encl.c:466
drivers/scsi/scsi_transport_sas.c:1075
Category 3: pos is used to determine if the list is empty (shouldn't need fixing)
drivers/perf/xgene_pmu.c:1487
drivers/media/pci/saa7134/saa7134-alsa.c:1232
arch/powerpc/sysdev/fsl_gtm.c:106
drivers/staging/greybus/audio_helper.c:135
drivers/misc/fastrpc.c:1363
drivers/dma/ppc4xx/adma.c:3048
drivers/dma/ppc4xx/adma.c:3060
Category 4: pos is used to determine if the break/goto was hit and the list is not empty
arch/arm/mach-mmp/sram.c:54
arch/arm/plat-pxa/ssp.c:54
arch/arm/plat-pxa/ssp.c:78
drivers/block/drbd/drbd_req.c:344
drivers/block/drbd/drbd_req.c:370
drivers/block/drbd/drbd_req.c:396
drivers/counter/counter-chrdev.c:145
drivers/counter/counter-chrdev.c:555
drivers/crypto/cavium/nitrox/nitrox_main.c:280
drivers/dma/ppc4xx/adma.c:954
drivers/firewire/core-transaction.c:93
drivers/firewire/core-transaction.c:963
drivers/firewire/sbp2.c:446
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2458
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2466
drivers/gpu/drm/drm_memory.c:79
drivers/gpu/drm/drm_mm.c:938
drivers/gpu/drm/drm_vm.c:160
drivers/gpu/drm/i915/gem/i915_gem_context.c:128
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:2471
drivers/gpu/drm/i915/gt/intel_ring.c:211
drivers/gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c:1168
drivers/gpu/drm/panfrost/panfrost_mmu.c:200
drivers/gpu/drm/scheduler/sched_main.c:1111
drivers/gpu/drm/ttm/ttm_bo.c:702
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:2600
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c:2624
drivers/infiniband/hw/hfi1/tid_rdma.c:1280
drivers/infiniband/hw/hfi1/tid_rdma.c:1280
drivers/infiniband/hw/mlx4/main.c:1933
drivers/media/dvb-frontends/mxl5xx.c:505
drivers/media/v4l2-core/v4l2-ctrls-api.c:1002
drivers/media/v4l2-core/v4l2-ctrls-api.c:986
drivers/misc/mei/interrupt.c:432
drivers/net/ethernet/mellanox/mlx4/alloc.c:273
drivers/net/ethernet/mellanox/mlx4/alloc.c:273
drivers/net/wireless/intel/ipw2x00/libipw_rx.c:1569
drivers/scsi/lpfc/lpfc_bsg.c:1218
drivers/staging/rtl8192e/rtl819x_TSProc.c:256
drivers/staging/rtl8192e/rtllib_rx.c:2638
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c:2370
drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c:255
drivers/usb/gadget/composite.c:2050
fs/proc/kcore.c:495
kernel/power/snapshot.c:637
kernel/power/snapshot.c:637
kernel/trace/ftrace.c:4570
kernel/trace/ftrace.c:4722
kernel/trace/trace_events.c:2285
drivers/net/ethernet/qlogic/qede/qede_filter.c:843
drivers/gpu/drm/gma500/oaktrail_lvds.c:120
drivers/power/supply/cpcap-battery.c:802
fs/cifs/smb2misc.c:161
kernel/debug/kdb/kdb_main.c:1031
kernel/debug/kdb/kdb_main.c:1038
kernel/debug/kdb/kdb_main.c:795
kernel/trace/trace_eprobe.c:670
net/9p/trans_xen.c:131
net/xfrm/xfrm_ipcomp.c:244
sound/soc/intel/catpt/pcm.c:362
sound/soc/sprd/sprd-mcdt.c:880
Category 5: legitimately uses head pos for cmp
net/ipv4/udp_tunnel_nic.c:849
drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c:487
drivers/scsi/wd719x.c:691
fs/f2fs/segment.c:368
net/tipc/socket.c:3752
net/tipc/name_table.c:970
Category 6: legitimately uses pos->member
drivers/net/dsa/sja1105/sja1105_vl.c:43
drivers/staging/android/ashmem.c:730
net/core/gro.c:38
fs/fs-writeback.c:437
mm/hugetlb.c:446
mm/compaction.c:1473
fs/locks.c:1130
drivers/net/ethernet/mellanox/mlx4/alloc.c:373
drivers/dma/at_xdmac.c:1583
arch/arm/mm/ioremap.c:107
arch/x86/kvm/mtrr.c:368
block/blk-mq.c:4466
drivers/dma/mv_xor.c:316
drivers/dma/mv_xor_v2.c:366
drivers/gpu/drm/nouveau/nvkm/core/mm.c:77
drivers/gpu/drm/nouveau/nvkm/subdev/timer/base.c:127
drivers/infiniband/hw/usnic/usnic_uiom.c:527
drivers/iommu/iommu.c:448
drivers/iommu/virtio-iommu.c:509
drivers/irqchip/irq-gic-v3-its.c:2093
drivers/md/dm-snap.c:542
drivers/net/ethernet/mellanox/mlx4/alloc.c:393
drivers/net/ethernet/sfc/rx_common.c:601
drivers/net/ethernet/ti/netcp_core.c:491
drivers/net/ethernet/ti/netcp_core.c:491
drivers/net/ethernet/ti/netcp_core.c:540
drivers/net/ethernet/ti/netcp_core.c:540
drivers/nvdimm/namespace_devs.c:1933
drivers/s390/char/tape_core.c:349
drivers/s390/cio/cmf.c:469
drivers/scsi/fcoe/fcoe.c:1885
drivers/scsi/lpfc/lpfc_sli.c:3498
drivers/target/target_core_user.c:400
drivers/usb/host/uhci-q.c:463
rivers/video/fbdev/core/fb_defio.c:139
drivers/xen/events/events_base.c:602
fs/dlm/lock.c:1315
fs/dlm/lock.c:1315
fs/f2fs/segment.c:4185
fs/locks.c:1238
fs/locks.c:1250
kernel/padata.c:397
kernel/trace/trace_output.c:717
net/tipc/group.c:392
net/tipc/monitor.c:416
sound/core/seq/seq_ports.c:152
sound/core/timer.c:1052
drivers/block/drbd/drbd_main.c:230
fs/locks.c:1130
drivers/gpu/drm/vc4/vc4_dsi.c:768
drivers/dma/ppc4xx/adma.c:2733
drivers/iio/industrialio-buffer.c:1128
drivers/net/ethernet/mellanox/mlxsw/spectrum_fid.c:535
drivers/net/ethernet/mellanox/mlxsw/spectrum_mr.c:655
drivers/net/ipvlan/ipvlan_main.c:47
drivers/staging/media/atomisp/pci/atomisp_acc.c:514
fs/gfs2/lops.c:681
fs/gfs2/lops.c:696
kernel/padata.c:656
net/netfilter/nf_tables_api.c:8291
arch/powerpc/platforms/cell/spufs/sched.c:387
drivers/dma/ppc4xx/adma.c:1436
drivers/net/wireless/ath/ath6kl/htc_mbox.c:107
drivers/dma/dw-edma/dw-edma-core.c:139
drivers/dma/dw-edma/dw-edma-core.c:159
drivers/net/ethernet/intel/i40e/i40e_ethtool.c:3966
drivers/scsi/lpfc/lpfc_sli.c:21042
drivers/net/ethernet/intel/ice/ice_sched.c:2808
drivers/net/ethernet/intel/ice/ice_sched.c:2811
drivers/net/wireless/st/cw1200/queue.c:120
arch/powerpc/kvm/book3s_hv_uvmem.c:370
drivers/gpu/drm/tilcdc/tilcdc_external.c:69
drivers/gpu/drm/stm/ltdc.c:559
sound/isa/cs423x/cs4236.c:520
drivers/media/usb/uvc/uvc_v4l2.c:896
drivers/iommu/msm_iommu.c:627
drivers/firmware/stratix10-svc.c:953
drivers/opp/debugfs.c:200
drivers/md/dm-mpath.c:1503
drivers/dma/ppc4xx/adma.c:3071
drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c:111
drivers/net/wireless/ath/ath10k/mac.c:510
drivers/s390/char/tty3270.c:1151
drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c:281
drivers/net/ethernet/intel/i40e/i40e_main.c:7590
drivers/staging/media/atomisp/pci/atomisp_cmd.c:959
drivers/staging/media/atomisp/pci/atomisp_cmd.c:979
drivers/staging/media/atomisp/pci/atomisp_cmd.c:998
drivers/watchdog/watchdog_pretimeout.c:215
fs/jfs/jfs_logmgr.c:892
kernel/workqueue.c:2946
drivers/media/usb/uvc/uvc_v4l2.c:885
drivers/gpu/drm/gma500/oaktrail_crtc.c:428
drivers/virt/acrn/ioreq.c:243
drivers/net/ethernet/mellanox/mlx5/core/eq.c:986
drivers/gpu/drm/omapdrm/omap_encoder.c:109
drivers/scsi/dc395x.c:3598
drivers/staging/media/atomisp/pci/atomisp_acc.c:508
drivers/net/dsa/bcm_sf2_cfp.c:577
drivers/perf/qcom_l2_pmu.c:764
drivers/gpu/drm/gma500/psb_intel_display.c:545
drivers/firmware/efi/vars.c:1092
drivers/staging/greybus/audio_codec.c:603
drivers/scsi/mpt3sas/mpt3sas_base.c:2016
Jakob Koschel (13):
list: introduce speculative safe list_for_each_entry()
scripts: coccinelle: adapt to find list_for_each_entry nospec issues
usb: remove the usage of the list iterator after the loop
vfio/mdev: remove the usage of the list iterator after the loop
drivers/perf: remove the usage of the list iterator after the loop
ARM: mmp: remove the usage of the list iterator after the loop
udp_tunnel: remove the usage of the list iterator after the loop
net: dsa: future proof usage of list iterator after the loop
drbd: future proof usage of list iterator after the loop
powerpc/spufs: future proof usage of list iterator after the loop
ath6kl: remove use of list iterator after the loop
staging: greybus: audio: Remove usage of list iterator after the loop
scsi: mpt3sas: comment about invalid usage of the list iterator
arch/arm/mach-mmp/sram.c | 7 ++++--
arch/powerpc/platforms/cell/spufs/sched.c | 2 ++
arch/x86/include/asm/barrier.h | 12 ++++++++++
drivers/block/drbd/drbd_main.c | 2 ++
drivers/net/dsa/sja1105/sja1105_vl.c | 2 ++
drivers/net/wireless/ath/ath6kl/htc_mbox.c | 2 +-
drivers/perf/xgene_pmu.c | 5 ++--
drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +-
drivers/staging/greybus/audio_codec.c | 4 ++--
drivers/usb/gadget/udc/gr_udc.c | 7 ++++--
drivers/vfio/mdev/mdev_core.c | 7 ++++--
include/linux/list.h | 3 ++-
include/linux/nospec.h | 16 +++++++++++++
net/ipv4/udp_tunnel_nic.c | 7 ++++--
.../coccinelle/iterators/use_after_iter.cocci | 24 -------------------
15 files changed, 63 insertions(+), 39 deletions(-)
base-commit: f71077a4d84bbe8c7b91b7db7c4ef815755ac5e3
--
2.25.1
With the speculative safe version of the list iterator req will be NULL
if the terminating condition was hit and needs to be reset to req
derived from the head, before calling list_for_each_entry_safe_from().
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/block/drbd/drbd_main.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
index 6f450816c4fa..d98205b46f59 100644
--- a/drivers/block/drbd/drbd_main.c
+++ b/drivers/block/drbd/drbd_main.c
@@ -227,6 +227,8 @@ void tl_release(struct drbd_connection *connection, unsigned int barrier_nr,
list_for_each_entry(req, &connection->transfer_log, tl_requests)
if (req->epoch == expect_epoch)
break;
+ if (!req)
+ req = list_entry(req, &connection->transfer_log, tl_requests);
list_for_each_entry_safe_from(req, r, &connection->transfer_log, tl_requests) {
if (req->epoch != expect_epoch)
break;
--
2.25.1
The usage of node->dev after the loop body is a legitimate type
confusion if the break was not hit. It will compare an undefined
memory location with dev that could potentially be equal. The value
of node->dev in this case could either be a random struct member of the
head element or an out-of-bounds value.
Therefore it is more safe to use the found variable. With the
introduction of speculative safe list iterator this check could be
replaced with if (!node).
Signed-off-by: Jakob Koschel <[email protected]>
---
net/ipv4/udp_tunnel_nic.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c
index b91003538d87..c47f9fb36d29 100644
--- a/net/ipv4/udp_tunnel_nic.c
+++ b/net/ipv4/udp_tunnel_nic.c
@@ -842,11 +842,14 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn)
*/
if (info->shared) {
struct udp_tunnel_nic_shared_node *node, *first;
+ bool found = false;
list_for_each_entry(node, &info->shared->devices, list)
- if (node->dev == dev)
+ if (node->dev == dev) {
+ found = true;
break;
- if (node->dev != dev)
+ }
+ if (!found)
return;
list_del(&node->list);
--
2.25.1
The list iterator module should not be used if data == NULL.
module can only old a legitimate value if data != NULL.
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/staging/greybus/audio_codec.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/greybus/audio_codec.c b/drivers/staging/greybus/audio_codec.c
index b589cf6b1d03..e19b91e7a72e 100644
--- a/drivers/staging/greybus/audio_codec.c
+++ b/drivers/staging/greybus/audio_codec.c
@@ -599,8 +599,8 @@ static int gbcodec_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
break;
}
if (!data) {
- dev_err(dai->dev, "%s:%s DATA connection missing\n",
- dai->name, module->name);
+ dev_err(dai->dev, "%s DATA connection missing\n",
+ dai->name);
mutex_unlock(&codec->lock);
return -ENODEV;
}
--
2.25.1
With the speculative safe version of the list iterator p will be NULL if
the terminating condition was hit and needs to be reset to p derived
from the head, before using p->list.
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/dsa/sja1105/sja1105_vl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index f5dca6a9b0f9..a4f2b95b09c4 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -40,6 +40,8 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
if (e->interval < p->interval)
break;
}
+ if (!p)
+ p = list_entry(p, &gating_cfg->entries, list);
list_add(&e->list, p->list.prev);
}
--
2.25.1
Le 17/02/2022 à 19:48, Jakob Koschel a écrit :
> The usage of node->dev after the loop body is a legitimate type
> confusion if the break was not hit. It will compare an undefined
> memory location with dev that could potentially be equal. The value
> of node->dev in this case could either be a random struct member of the
> head element or an out-of-bounds value.
>
> Therefore it is more safe to use the found variable. With the
> introduction of speculative safe list iterator this check could be
> replaced with if (!node).
>
> Signed-off-by: Jakob Koschel <[email protected]>
> ---
> net/ipv4/udp_tunnel_nic.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c
> index b91003538d87..c47f9fb36d29 100644
> --- a/net/ipv4/udp_tunnel_nic.c
> +++ b/net/ipv4/udp_tunnel_nic.c
> @@ -842,11 +842,14 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn)
> */
> if (info->shared) {
> struct udp_tunnel_nic_shared_node *node, *first;
> + bool found = false;
>
> list_for_each_entry(node, &info->shared->devices, list)
> - if (node->dev == dev)
> + if (node->dev == dev) {
> + found = true;
> break;
> - if (node->dev != dev)
> + }
> + if (!found)
> return;
>
> list_del(&node->list);
Hi,
just in case, see Dan Carpeter's patch for the same issue with another
fix at:
https://lore.kernel.org/kernel-janitors/20220222134251.GA2271@kili/
CJ
On Wed, Feb 23, 2022 at 09:00:36PM +0100, Christophe JAILLET wrote:
> Le 17/02/2022 ? 19:48, Jakob Koschel a ?crit?:
> > The usage of node->dev after the loop body is a legitimate type
> > confusion if the break was not hit. It will compare an undefined
> > memory location with dev that could potentially be equal. The value
> > of node->dev in this case could either be a random struct member of the
> > head element or an out-of-bounds value.
> >
> > Therefore it is more safe to use the found variable. With the
> > introduction of speculative safe list iterator this check could be
> > replaced with if (!node).
> >
> > Signed-off-by: Jakob Koschel <[email protected]>
> > ---
> > net/ipv4/udp_tunnel_nic.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c
> > index b91003538d87..c47f9fb36d29 100644
> > --- a/net/ipv4/udp_tunnel_nic.c
> > +++ b/net/ipv4/udp_tunnel_nic.c
> > @@ -842,11 +842,14 @@ udp_tunnel_nic_unregister(struct net_device *dev, struct udp_tunnel_nic *utn)
> > */
> > if (info->shared) {
> > struct udp_tunnel_nic_shared_node *node, *first;
> > + bool found = false;
> > list_for_each_entry(node, &info->shared->devices, list)
> > - if (node->dev == dev)
> > + if (node->dev == dev) {
> > + found = true;
> > break;
> > - if (node->dev != dev)
> > + }
> > + if (!found)
> > return;
> > list_del(&node->list);
>
> Hi,
>
> just in case, see Dan Carpeter's patch for the same issue with another fix
> at:
> https://lore.kernel.org/kernel-janitors/20220222134251.GA2271@kili/
Yeah. My patch was already applied.
I've had an unpublished Smatch check for this for a while but I've been
re-writing it recently to make it more generic so that it worked for
all the different list_for_each type macros. I'm going to publish it
soon.
Of course, all the real bugs are fixed so the remaining warnings are
false positives.
regards,
dan carpenter