History

kcov: fix data corruption and race conditions on PREEMPT_RT

syzbot is reporting KCOV state corruption on PREEMPT_RT kernels, for the temporary storage used for saving/restoring remote KCOV state is currently allocated as the per-CPU area. On PREEMPT_RT kernels, softirq handlers run as preemptible task threads (e.g., ksoftirqd). If a softirq context preempts a task running a remote KCOV session, it safely saves the task's state into the per-CPU area. However, if that softirq thread is subsequently preempted by a higher- priority softirq thread on the same CPU, the second softirq will overwrite the same per-CPU area, permanently destroying the original task's KCOV state. Fix this data corruption by moving the temporary storage from the per-CPU area to the per-thread area. Since each softirq thread now owns its own task context, nested softirq preemption no longer causes data overwrites. Note that while the temporary storage is now on a per-thread basis, the per-CPU kcov_percpu_data.lock must be retained, for we need to ensure that kcov_remote_start() and kcov_remote_stop() operate atomically without racing against asynchronous interrupts that manipulate the current task's KCOV state. It is likely that GFP_KERNEL allocation by vmalloc_node() in kcov_init() has already called panic() before returning NULL, for there will be no OOM-killable userspace processes when __init function of built-in module runs. But this patch also fixes crashing the kernel when vmalloc_node() in kcov_init() returned NULL, for kcov_init() left per-CPU irq_area == NULL but kcov_remote_start() depends on per-CPU irq_area != NULL, resulting in (1) doing vmalloc() in kcov_remote_start() despite !in_task() context (2) out-of-array-bounds access if (1) succeeded but kcov->remote_size < CONFIG_KCOV_IRQ_AREA_SIZE (3) always leak memory allocated by (1), eventually killing all OOM-killable userspace processes problems.

Tetsuo Handa committed 4 months ago

Commit 2eed77f1 parent 0cdc7dd3 files · +58 −45Browse files

Showing the changes to kernel (1 of 3 files). Show all

Unified · Split

kernel/kcov.c+47−43
@@ -86,17 +86,12 @@ struct kcov_remote { · 85 unmodified lines
86 86
87static DEFINE_SPINLOCK(kcov_remote_lock);87static DEFINE_SPINLOCK(kcov_remote_lock);
88static DEFINE_HASHTABLE(kcov_remote_map, 4);88static DEFINE_HASHTABLE(kcov_remote_map, 4);
89static struct list_head kcov_remote_areas = LIST_HEAD_INIT(kcov_remote_areas);89static struct list_head kcov_remote_areas[2] = {
90 LIST_HEAD_INIT(kcov_remote_areas[0]), LIST_HEAD_INIT(kcov_remote_areas[1])
91};
90 92
91struct kcov_percpu_data {93struct kcov_percpu_data {
92 void *irq_area;
93 local_lock_t lock;94 local_lock_t lock;
94
95 unsigned int saved_mode;
96 unsigned int saved_size;
97 void *saved_area;
98 struct kcov *saved_kcov;
99 int saved_sequence;
100};95};
101 96
102static DEFINE_PER_CPU(struct kcov_percpu_data, kcov_percpu_data) = {97static DEFINE_PER_CPU(struct kcov_percpu_data, kcov_percpu_data) = {
@@ -132,12 +127,13 @@ static struct kcov_remote *kcov_remote_add(struct kcov *kcov, u64 handle) · 29 unmodified lines
132}127}
133 128
134/* Must be called with kcov_remote_lock locked. */129/* Must be called with kcov_remote_lock locked. */
135static struct kcov_remote_area *kcov_remote_area_get(unsigned int size)130static struct kcov_remote_area *kcov_remote_area_get(unsigned int size, bool irq)
136{131{
137 struct kcov_remote_area *area;132 struct kcov_remote_area *area;
138 struct list_head *pos;133 struct list_head *pos;
134 struct list_head *list = &kcov_remote_areas[irq];
139 135
140 list_for_each(pos, &kcov_remote_areas) {136 list_for_each(pos, list) {
141 area = list_entry(pos, struct kcov_remote_area, list);137 area = list_entry(pos, struct kcov_remote_area, list);
142 if (area->size == size) {138 if (area->size == size) {
143 list_del(&area->list);139 list_del(&area->list);
@@ -149,11 +145,11 @@ static struct kcov_remote_area *kcov_remote_area_get(unsigned int size) · 5 unmodified lines
149 145
150/* Must be called with kcov_remote_lock locked. */146/* Must be called with kcov_remote_lock locked. */
151static void kcov_remote_area_put(struct kcov_remote_area *area,147static void kcov_remote_area_put(struct kcov_remote_area *area,
152 unsigned int size)148 unsigned int size, bool irq)
153{149{
154 INIT_LIST_HEAD(&area->list);150 INIT_LIST_HEAD(&area->list);
155 area->size = size;151 area->size = size;
156 list_add(&area->list, &kcov_remote_areas);152 list_add(&area->list, &kcov_remote_areas[irq]);
157 /*153 /*
158 * KMSAN doesn't instrument this file, so it may not know area->list154 * KMSAN doesn't instrument this file, so it may not know area->list
159 * is initialized. Unpoison it explicitly to avoid reports in155 * is initialized. Unpoison it explicitly to avoid reports in
@@ -390,6 +386,12 @@ void kcov_task_init(struct task_struct *t) · 230 unmodified lines
390 kcov_task_reset(t);386 kcov_task_reset(t);
391 t->kcov_remote = NULL;387 t->kcov_remote = NULL;
392 t->kcov_handle = current->kcov_handle;388 t->kcov_handle = current->kcov_handle;
389 t->kcov_softirq = 0;
390 t->kcov_saved_mode = 0;
391 t->kcov_saved_size = 0;
392 t->kcov_saved_area = NULL;
393 t->kcov_saved_kcov = NULL;
394 t->kcov_saved_sequence = 0;
393}395}
394 396
395static void kcov_reset(struct kcov *kcov)397static void kcov_reset(struct kcov *kcov)
@@ -836,17 +838,16 @@ static inline bool kcov_mode_enabled(unsigned int mode) · 440 unmodified lines
836static void kcov_remote_softirq_start(struct task_struct *t)838static void kcov_remote_softirq_start(struct task_struct *t)
837 __must_hold(&kcov_percpu_data.lock)839 __must_hold(&kcov_percpu_data.lock)
838{840{
839 struct kcov_percpu_data *data = this_cpu_ptr(&kcov_percpu_data);
840 unsigned int mode;841 unsigned int mode;
841 842
842 mode = READ_ONCE(t->kcov_mode);843 mode = READ_ONCE(t->kcov_mode);
843 barrier();844 barrier();
844 if (kcov_mode_enabled(mode)) {845 if (kcov_mode_enabled(mode)) {
845 data->saved_mode = mode;846 t->kcov_saved_mode = mode;
846 data->saved_size = t->kcov_size;847 t->kcov_saved_size = t->kcov_size;
847 data->saved_area = t->kcov_area;848 t->kcov_saved_area = t->kcov_area;
848 data->saved_sequence = t->kcov_sequence;849 t->kcov_saved_sequence = t->kcov_sequence;
849 data->saved_kcov = t->kcov;850 t->kcov_saved_kcov = t->kcov;
850 kcov_stop(t);851 kcov_stop(t);
851 }852 }
852}853}
@@ -854,17 +855,15 @@ static void kcov_remote_softirq_start(struct task_struct *t) · 1 unmodified line
854static void kcov_remote_softirq_stop(struct task_struct *t)855static void kcov_remote_softirq_stop(struct task_struct *t)
855 __must_hold(&kcov_percpu_data.lock)856 __must_hold(&kcov_percpu_data.lock)
856{857{
857 struct kcov_percpu_data *data = this_cpu_ptr(&kcov_percpu_data);858 if (t->kcov_saved_kcov) {
858 859 kcov_start(t, t->kcov_saved_kcov, t->kcov_saved_size,
859 if (data->saved_kcov) {860 t->kcov_saved_area, t->kcov_saved_mode,
860 kcov_start(t, data->saved_kcov, data->saved_size,861 t->kcov_saved_sequence);
861 data->saved_area, data->saved_mode,862 t->kcov_saved_mode = 0;
862 data->saved_sequence);863 t->kcov_saved_size = 0;
863 data->saved_mode = 0;864 t->kcov_saved_area = NULL;
864 data->saved_size = 0;865 t->kcov_saved_sequence = 0;
865 data->saved_area = NULL;866 t->kcov_saved_kcov = NULL;
866 data->saved_sequence = 0;
867 data->saved_kcov = NULL;
868 }867 }
869}868}
870 869
@@ -927,17 +926,17 @@ void kcov_remote_start(u64 handle) · 56 unmodified lines
927 sequence = kcov->sequence;926 sequence = kcov->sequence;
928 if (in_task()) {927 if (in_task()) {
929 size = kcov->remote_size;928 size = kcov->remote_size;
930 area = kcov_remote_area_get(size);929 area = kcov_remote_area_get(size, false);
931 } else {930 } else {
932 size = CONFIG_KCOV_IRQ_AREA_SIZE;931 size = CONFIG_KCOV_IRQ_AREA_SIZE;
933 area = this_cpu_ptr(&kcov_percpu_data)->irq_area;932 area = kcov_remote_area_get(size, true);
934 }933 }
935 spin_unlock(&kcov_remote_lock);934 spin_unlock(&kcov_remote_lock);
936 935
937 /* Can only happen when in_task(). */936 /* Allocate new buffer if we can sleep. */
938 if (!area) {937 if (!area) {
939 local_unlock_irqrestore(&kcov_percpu_data.lock, flags);938 local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
940 area = vmalloc(size * sizeof(unsigned long));939 area = in_task() ? vmalloc(size * sizeof(unsigned long)) : NULL;
941 if (!area) {940 if (!area) {
942 kcov_put(kcov);941 kcov_put(kcov);
943 return;942 return;
@@ -1079,11 +1078,9 @@ void kcov_remote_stop(void) · 135 unmodified lines
1079 kcov_move_area(kcov->mode, kcov->area, kcov->size, area);1078 kcov_move_area(kcov->mode, kcov->area, kcov->size, area);
1080 spin_unlock(&kcov->lock);1079 spin_unlock(&kcov->lock);
1081 1080
1082 if (in_task()) {1081 spin_lock(&kcov_remote_lock);
1083 spin_lock(&kcov_remote_lock);1082 kcov_remote_area_put(area, size, !in_task());
1084 kcov_remote_area_put(area, size);1083 spin_unlock(&kcov_remote_lock);
1085 spin_unlock(&kcov_remote_lock);
1086 }
1087 1084
1088 local_unlock_irqrestore(&kcov_percpu_data.lock, flags);1085 local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
1089 1086
@@ -1129,14 +1126,21 @@ static void __init selftest(void) · 39 unmodified lines
1129 1126
1130static int __init kcov_init(void)1127static int __init kcov_init(void)
1131{1128{
1132 int cpu;1129 int cpu = num_possible_cpus();
1130
1131#ifdef CONFIG_PREEMPT_RT
1132 /* Allocate some extra buffers in order to prepare for softirq preemption. */
1133 cpu = cpu >= 4 ? cpu * 2 : cpu + 4;
1134#endif
1135 while (cpu--) {
1136 void *area = vmalloc(CONFIG_KCOV_IRQ_AREA_SIZE * sizeof(unsigned long));
1137 unsigned long flags;
1133 1138
1134 for_each_possible_cpu(cpu) {
1135 void *area = vmalloc_node(CONFIG_KCOV_IRQ_AREA_SIZE *
1136 sizeof(unsigned long), cpu_to_node(cpu));
1137 if (!area)1139 if (!area)
1138 return -ENOMEM;1140 return -ENOMEM;
1139 per_cpu_ptr(&kcov_percpu_data, cpu)->irq_area = area;1141 spin_lock_irqsave(&kcov_remote_lock, flags);
1142 kcov_remote_area_put(area, CONFIG_KCOV_IRQ_AREA_SIZE, true);
1143 spin_unlock_irqrestore(&kcov_remote_lock, flags);
1140 }1144 }
1141 1145
1142 /*1146 /*