ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities

PATCH In review Gustavo A. R. Silva 2 participants 5 messages Dec 19, 2018 – Dec 20, 2018 Sound

Share & tools

Gustavo A. R. Silva

author

2018-12-19 23:31 UTC #0 lore raw

PATCH 1/1 ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities

header->number is indirectly controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.

This issue was detected with the help of Smatch:

sound/isa/wavefront/wavefront_synth.c:792 wavefront_send_patch() warn: potential spectre issue 'dev->patch_status' [w] (local cap) sound/isa/wavefront/wavefront_synth.c:819 wavefront_send_program() warn: potential spectre issue 'dev->prog_status' [w] (local cap) sound/isa/wavefront/wavefront_synth.c:1197 wavefront_send_alias() warn: potential spectre issue 'dev->sample_status' [w] sound/isa/wavefront/wavefront_synth.c:1248 wavefront_send_multisample() warn: potential spectre issue 'dev->sample_status' [w] sound/isa/wavefront/wavefront_synth.c:1548 wavefront_synth_control() warn: potential spectre issue 'dev->sample_status' [r] (local cap)

Fix this by sanitizing header->number before using it to index dev->patch_status, dev->prog_status and dev->sample_status.

Notice that given that speculation windows are large, the policy is to kill the speculation on the first load and not worry if it can be completed with a dependent load/store [1].

[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2

Cc [email protected]

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 sound/isa/wavefront/wavefront_synth.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

sound/isa/wavefront/wavefront_synth.c +15 -3

index 0b1e4b34b299..80196a533a36 100644
@@ -31,6 +31,7 @@
 #include <linux/moduleparam.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/nospec.h>
 #include <sound/core.h>
 #include <sound/snd_wavefront.h>
 #include <sound/initval.h>

@@ -788,7 +789,8 @@ wavefront_send_patch (snd_wavefront_t *dev, wavefront_patch_info *header)
 
 	if (header->number >= ARRAY_SIZE(dev->patch_status))
 		return -EINVAL;
-
+	header->number = array_index_nospec(header->number,
+					    ARRAY_SIZE(dev->patch_status));
 	dev->patch_status[header->number] |= WF_SLOT_FILLED;
 
 	bptr = buf;

@@ -815,7 +817,8 @@ wavefront_send_program (snd_wavefront_t *dev, wavefront_patch_info *header)
 
 	if (header->number >= ARRAY_SIZE(dev->prog_status))
 		return -EINVAL;
-
+	header->number = array_index_nospec(header->number,
+					    ARRAY_SIZE(dev->prog_status));
 	dev->prog_status[header->number] = WF_SLOT_USED;
 
 	/* XXX need to zero existing SLOT_USED bit for program_status[i]

@@ -1194,6 +1197,10 @@ wavefront_send_alias (snd_wavefront_t *dev, wavefront_patch_info *header)
 		return -EIO;
 	}
 
+	if (header->number >= ARRAY_SIZE(dev->sample_status))
+		return -EINVAL;
+	header->number = array_index_nospec(header->number,
+					    ARRAY_SIZE(dev->sample_status));
 	dev->sample_status[header->number] = (WF_SLOT_FILLED|WF_ST_ALIAS);
 
 	return (0);

@@ -1245,6 +1252,10 @@ wavefront_send_multisample (snd_wavefront_t *dev, wavefront_patch_info *header)
 		return -EIO;
 	}
 
+	if (header->number >= ARRAY_SIZE(dev->sample_status))
+		return -EINVAL;
+	header->number = array_index_nospec(header->number,
+					    ARRAY_SIZE(dev->sample_status));
 	dev->sample_status[header->number] = (WF_SLOT_FILLED|WF_ST_MULTISAMPLE);
 
 	kfree(msample_hdr);

@@ -1539,12 +1550,13 @@ wavefront_synth_control (snd_wavefront_card_t *acard,
 
 	case WFC_IDENTIFY_SLOT_TYPE:
 		i = wc->wbuf[0] | (wc->wbuf[1] << 7);
-		if (i <0 || i >= WF_MAX_SAMPLE) {
+		if (i < 0 || i >= WF_MAX_SAMPLE) {
 			snd_printk ("invalid slot ID %d\n",
 				i);
 			wc->status = EINVAL;
 			return -EINVAL;
 		}
+		i = array_index_nospec(i, WF_MAX_SAMPLE);
 		wc->rbuf[0] = dev->sample_status[i];
 		wc->status = 0;
 		return 0;

Signature · 1 line

2.20.1

Takashi Iwai

2018-12-20 08:11 UTC #1 lore raw

Quoted 15 lines · Gustavo A. R. Silva

header->number is indirectly controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.
This issue was detected with the help of Smatch:
sound/isa/wavefront/wavefront_synth.c:792 wavefront_send_patch() warn: potential spectre issue 'dev->patch_status' [w] (local cap) sound/isa/wavefront/wavefront_synth.c:819 wavefront_send_program() warn: potential spectre issue 'dev->prog_status' [w] (local cap) sound/isa/wavefront/wavefront_synth.c:1197 wavefront_send_alias() warn: potential spectre issue 'dev->sample_status' [w] sound/isa/wavefront/wavefront_synth.c:1248 wavefront_send_multisample() warn: potential spectre issue 'dev->sample_status' [w] sound/isa/wavefront/wavefront_synth.c:1548 wavefront_synth_control() warn: potential spectre issue 'dev->sample_status' [r] (local cap)
Fix this by sanitizing header->number before using it to index dev->patch_status, dev->prog_status and dev->sample_status.

Quoted 8 lines

Notice that given that speculation windows are large, the policy is to kill the speculation on the first load and not worry if it can be completed with a dependent load/store [1].
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Cc [email protected]
Signed-off-by Gustavo A. R. Silva <[email protected]>

Is there any platform with ISA slot that suffers from Spectre?

thanks,

Takashi

Gustavo A. R. Silva

author

2018-12-20 17:13 UTC #2 lore raw

Quoted 21 lines · Takashi Iwai

Quoted 19 lines · Gustavo A. R. Silva
header->number is indirectly controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.
This issue was detected with the help of Smatch:
sound/isa/wavefront/wavefront_synth.c:792 wavefront_send_patch() warn: potential spectre issue 'dev->patch_status' [w] (local cap) sound/isa/wavefront/wavefront_synth.c:819 wavefront_send_program() warn: potential spectre issue 'dev->prog_status' [w] (local cap) sound/isa/wavefront/wavefront_synth.c:1197 wavefront_send_alias() warn: potential spectre issue 'dev->sample_status' [w] sound/isa/wavefront/wavefront_synth.c:1248 wavefront_send_multisample() warn: potential spectre issue 'dev->sample_status' [w] sound/isa/wavefront/wavefront_synth.c:1548 wavefront_synth_control() warn: potential spectre issue 'dev->sample_status' [r] (local cap)
Fix this by sanitizing header->number before using it to index dev->patch_status, dev->prog_status and dev->sample_status.
Notice that given that speculation windows are large, the policy is to kill the speculation on the first load and not worry if it can be completed with a dependent load/store [1].

Quoted 8 lines

Quoted 4 lines
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2
Cc [email protected]
Signed-off-by Gustavo A. R. Silva <[email protected]>
Is there any platform with ISA slot that suffers from Spectre?

Do you mean 'any other'?

If so, yeah, I just spotted some Spectre vulns in sound/isa/sb/emu8000.c

I'll send a patch for that.

Thanks
--
Gustavo

Takashi Iwai

2018-12-20 17:35 UTC #3 lore raw

Quoted 25 lines · Gustavo A. R. Silva

Quoted 23 lines · Takashi Iwai
Quoted 21 lines · Gustavo A. R. Silva
header->number is indirectly controlled by user-space, hence leading to a potential exploitation of the Spectre variant 1 vulnerability.
This issue was detected with the help of Smatch:
sound/isa/wavefront/wavefront_synth.c:792 wavefront_send_patch() warn: potential spectre issue 'dev->patch_status' [w] (local cap) sound/isa/wavefront/wavefront_synth.c:819 wavefront_send_program() warn: potential spectre issue 'dev->prog_status' [w] (local cap) sound/isa/wavefront/wavefront_synth.c:1197 wavefront_send_alias() warn: potential spectre issue 'dev->sample_status' [w] sound/isa/wavefront/wavefront_synth.c:1248 wavefront_send_multisample() warn: potential spectre issue 'dev->sample_status' [w] sound/isa/wavefront/wavefront_synth.c:1548 wavefront_synth_control() warn: potential spectre issue 'dev->sample_status' [r] (local cap)
Fix this by sanitizing header->number before using it to index dev->patch_status, dev->prog_status and dev->sample_status.
Notice that given that speculation windows are large, the policy is to kill the speculation on the first load and not worry if it can be completed with a dependent load/store [1].
[1] https://marc.info/?l=linux-kernel&m=152449131114778&w=2

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Cc [email protected]
Signed-off-by Gustavo A. R. Silva <[email protected]>
Is there any platform with ISA slot that suffers from Spectre?
Do you mean 'any other'?

Well, no, my question is whether it makes sense to patch the code path for such ISA drivers. Spectre seems applicable since the model around 2006 or so, and ISA slot has been already dead for very long time. And yet with this minor board... I bet no one hits this in the world.

thanks,

Takashi

Gustavo A. R. Silva

author

2018-12-20 17:51 UTC #4 lore raw

Quoted 7 lines · Takashi Iwai

Well, no, my question is whether it makes sense to patch the code path for such ISA drivers. Spectre seems applicable since the model around 2006 or so, and ISA slot has been already dead for very long time. And yet with this minor board... I bet no one hits this in the world.

Oh, I see. I'm not stopping to analyze all the context around a driver other than the code itself. So, thanks for pointing this out. I won't touch those drivers from now on.

Thanks
--
Gustavo