linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities
@ 2018-12-19 23:31 Gustavo A. R. Silva
  2018-12-20  8:11 ` Takashi Iwai
  0 siblings, 1 reply; 9+ messages in thread
From: Gustavo A. R. Silva @ 2018-12-19 23:31 UTC (permalink / raw)
  To: Jaroslav Kysela, Takashi Iwai
  Cc: alsa-devel, linux-kernel, 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

Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 sound/isa/wavefront/wavefront_synth.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/sound/isa/wavefront/wavefront_synth.c b/sound/isa/wavefront/wavefront_synth.c
index 0b1e4b34b299..80196a533a36 100644
--- a/sound/isa/wavefront/wavefront_synth.c
+++ b/sound/isa/wavefront/wavefront_synth.c
@@ -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;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities
  2018-12-19 23:31 [PATCH] ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities Gustavo A. R. Silva
@ 2018-12-20  8:11 ` Takashi Iwai
  2018-12-20 17:13   ` Gustavo A. R. Silva
  0 siblings, 1 reply; 9+ messages in thread
From: Takashi Iwai @ 2018-12-20  8:11 UTC (permalink / raw)
  To:  Gustavo A. R. Silva ; +Cc: Jaroslav Kysela, alsa-devel, linux-kernel

On Thu, 20 Dec 2018 00:31:43 +0100,
 Gustavo A. R. Silva  wrote:
> 
> 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: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>

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


thanks,

Takashi

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities
  2018-12-20  8:11 ` Takashi Iwai
@ 2018-12-20 17:13   ` Gustavo A. R. Silva
  2018-12-20 17:35     ` Takashi Iwai
  0 siblings, 1 reply; 9+ messages in thread
From: Gustavo A. R. Silva @ 2018-12-20 17:13 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: Jaroslav Kysela, alsa-devel, linux-kernel



On 12/20/18 2:11 AM, Takashi Iwai wrote:
> On Thu, 20 Dec 2018 00:31:43 +0100,
>   Gustavo A. R. Silva  wrote:
>>
>> 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: stable@vger.kernel.org
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> 
> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities
  2018-12-20 17:13   ` Gustavo A. R. Silva
@ 2018-12-20 17:35     ` Takashi Iwai
  2018-12-20 17:51       ` Gustavo A. R. Silva
  0 siblings, 1 reply; 9+ messages in thread
From: Takashi Iwai @ 2018-12-20 17:35 UTC (permalink / raw)
  To: Gustavo A. R. Silva; +Cc: Jaroslav Kysela, alsa-devel, linux-kernel

On Thu, 20 Dec 2018 18:13:31 +0100,
Gustavo A. R. Silva wrote:
> 
> On 12/20/18 2:11 AM, Takashi Iwai wrote:
> > On Thu, 20 Dec 2018 00:31:43 +0100,
> >   Gustavo A. R. Silva  wrote:
> >>
> >> 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: stable@vger.kernel.org
> >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> >
> > 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities
  2018-12-20 17:35     ` Takashi Iwai
@ 2018-12-20 17:51       ` Gustavo A. R. Silva
  0 siblings, 0 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2018-12-20 17:51 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: Jaroslav Kysela, alsa-devel, linux-kernel



On 12/20/18 11:35 AM, Takashi Iwai wrote:

> 
> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities
  2019-04-15 22:34   ` Takashi Iwai
@ 2019-04-15 22:40     ` Gustavo A. R. Silva
  0 siblings, 0 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2019-04-15 22:40 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: Jaroslav Kysela, alsa-devel, linux-kernel



On 4/15/19 5:34 PM, Takashi Iwai wrote:
> On Mon, 15 Apr 2019 21:35:17 +0200,
>  Gustavo A. R. Silva  wrote:
>>
>> Hi all,
>>
>> Friendly ping:
>>
>> Who can take this?
> 
> All platforms that support ISA boards are so old and they don't suffer
> from Spectre at all.
> 

Oh okay.

I'll take this info into account for future changes.

Thanks
--
Gustavo

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities
  2019-04-15 19:35 ` Gustavo A. R. Silva
@ 2019-04-15 22:34   ` Takashi Iwai
  2019-04-15 22:40     ` Gustavo A. R. Silva
  0 siblings, 1 reply; 9+ messages in thread
From: Takashi Iwai @ 2019-04-15 22:34 UTC (permalink / raw)
  To:  Gustavo A. R. Silva ; +Cc: Jaroslav Kysela, alsa-devel, linux-kernel

On Mon, 15 Apr 2019 21:35:17 +0200,
 Gustavo A. R. Silva  wrote:
> 
> Hi all,
> 
> Friendly ping:
> 
> Who can take this?

All platforms that support ISA boards are so old and they don't suffer
from Spectre at all.


thanks,

Takashi


> 
> Thanks
> --
> Gustavo
> 
> On 3/26/19 1:32 PM, Gustavo A. R. Silva wrote:
> > header->number and i are 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 and i before using them to index
> > dev->patch_status, dev->prog_status, 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://lore.kernel.org/lkml/20180423164740.GY17484@dhcp22.suse.cz/
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > ---
> >  sound/isa/wavefront/wavefront_synth.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/sound/isa/wavefront/wavefront_synth.c b/sound/isa/wavefront/wavefront_synth.c
> > index 0b1e4b34b299..a78e125dfdf9 100644
> > --- a/sound/isa/wavefront/wavefront_synth.c
> > +++ b/sound/isa/wavefront/wavefront_synth.c
> > @@ -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,6 +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;
> >  
> > @@ -815,6 +818,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;
> >  
> > @@ -1194,6 +1199,11 @@ 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 +1255,12 @@ wavefront_send_multisample (snd_wavefront_t *dev, wavefront_patch_info *header)
> >  		return -EIO;
> >  	}
> >  
> > +	if (header->number >= ARRAY_SIZE(dev->sample_status)) {
> > +		kfree(msample_hdr);
> > +		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);
> > @@ -1545,6 +1561,7 @@ wavefront_synth_control (snd_wavefront_card_t *acard,
> >  			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;
> > 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities
  2019-03-26 18:32 Gustavo A. R. Silva
@ 2019-04-15 19:35 ` Gustavo A. R. Silva
  2019-04-15 22:34   ` Takashi Iwai
  0 siblings, 1 reply; 9+ messages in thread
From: Gustavo A. R. Silva @ 2019-04-15 19:35 UTC (permalink / raw)
  To: Jaroslav Kysela, Takashi Iwai; +Cc: alsa-devel, linux-kernel

Hi all,

Friendly ping:

Who can take this?

Thanks
--
Gustavo

On 3/26/19 1:32 PM, Gustavo A. R. Silva wrote:
> header->number and i are 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 and i before using them to index
> dev->patch_status, dev->prog_status, 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://lore.kernel.org/lkml/20180423164740.GY17484@dhcp22.suse.cz/
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  sound/isa/wavefront/wavefront_synth.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/sound/isa/wavefront/wavefront_synth.c b/sound/isa/wavefront/wavefront_synth.c
> index 0b1e4b34b299..a78e125dfdf9 100644
> --- a/sound/isa/wavefront/wavefront_synth.c
> +++ b/sound/isa/wavefront/wavefront_synth.c
> @@ -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,6 +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;
>  
> @@ -815,6 +818,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;
>  
> @@ -1194,6 +1199,11 @@ 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 +1255,12 @@ wavefront_send_multisample (snd_wavefront_t *dev, wavefront_patch_info *header)
>  		return -EIO;
>  	}
>  
> +	if (header->number >= ARRAY_SIZE(dev->sample_status)) {
> +		kfree(msample_hdr);
> +		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);
> @@ -1545,6 +1561,7 @@ wavefront_synth_control (snd_wavefront_card_t *acard,
>  			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;
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities
@ 2019-03-26 18:32 Gustavo A. R. Silva
  2019-04-15 19:35 ` Gustavo A. R. Silva
  0 siblings, 1 reply; 9+ messages in thread
From: Gustavo A. R. Silva @ 2019-03-26 18:32 UTC (permalink / raw)
  To: Jaroslav Kysela, Takashi Iwai
  Cc: alsa-devel, linux-kernel, Gustavo A. R. Silva

header->number and i are 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 and i before using them to index
dev->patch_status, dev->prog_status, 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://lore.kernel.org/lkml/20180423164740.GY17484@dhcp22.suse.cz/

Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 sound/isa/wavefront/wavefront_synth.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/sound/isa/wavefront/wavefront_synth.c b/sound/isa/wavefront/wavefront_synth.c
index 0b1e4b34b299..a78e125dfdf9 100644
--- a/sound/isa/wavefront/wavefront_synth.c
+++ b/sound/isa/wavefront/wavefront_synth.c
@@ -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,6 +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;
 
@@ -815,6 +818,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;
 
@@ -1194,6 +1199,11 @@ 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 +1255,12 @@ wavefront_send_multisample (snd_wavefront_t *dev, wavefront_patch_info *header)
 		return -EIO;
 	}
 
+	if (header->number >= ARRAY_SIZE(dev->sample_status)) {
+		kfree(msample_hdr);
+		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);
@@ -1545,6 +1561,7 @@ wavefront_synth_control (snd_wavefront_card_t *acard,
 			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;
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-04-15 22:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-19 23:31 [PATCH] ALSA: isa/wavefront: Fix potential Spectre v1 vulnerabilities Gustavo A. R. Silva
2018-12-20  8:11 ` Takashi Iwai
2018-12-20 17:13   ` Gustavo A. R. Silva
2018-12-20 17:35     ` Takashi Iwai
2018-12-20 17:51       ` Gustavo A. R. Silva
2019-03-26 18:32 Gustavo A. R. Silva
2019-04-15 19:35 ` Gustavo A. R. Silva
2019-04-15 22:34   ` Takashi Iwai
2019-04-15 22:40     ` Gustavo A. R. Silva

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).