net: atm: Spectre v1 fix introduced bug in bcb964012d1b in -stable

net: atm: Spectre v1 fix introduced bug in bcb964012d1b in -stable

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]


In lecd_attach, if arg is < 0, it was treated as 0. Spectre v1 fix
changed that. Bug does not exist in mainline AFAICT.

Signed-off-by: Pavel Machek <pavel@denx.de>
# for 4.19.y

diff --git a/net/atm/lec.c b/net/atm/lec.c
index ad4f829193f0..ed279cd912f4 100644
--- a/net/atm/lec.c
+++ b/net/atm/lec.c
@@ -731,7 +731,7 @@ static int lecd_attach(struct atm_vcc *vcc, int arg)
 		i = arg;
 	if (arg >= MAX_LEC_ITF)
 		return -EINVAL;
-	i = array_index_nospec(arg, MAX_LEC_ITF);
+	i = array_index_nospec(i, MAX_LEC_ITF);
 	if (!dev_lec[i]) {
 		int size;
 
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: net: atm: Spectre v1 fix introduced bug in bcb964012d1b in -stable

[Greg KH]

On Mon, May 20, 2019 at 02:40:14PM +0200, Pavel Machek wrote:
> 
> In lecd_attach, if arg is < 0, it was treated as 0. Spectre v1 fix
> changed that. Bug does not exist in mainline AFAICT.
> 
> Signed-off-by: Pavel Machek <pavel@denx.de>
> # for 4.19.y
> 
> diff --git a/net/atm/lec.c b/net/atm/lec.c
> index ad4f829193f0..ed279cd912f4 100644
> --- a/net/atm/lec.c
> +++ b/net/atm/lec.c
> @@ -731,7 +731,7 @@ static int lecd_attach(struct atm_vcc *vcc, int arg)
>  		i = arg;
>  	if (arg >= MAX_LEC_ITF)
>  		return -EINVAL;
> -	i = array_index_nospec(arg, MAX_LEC_ITF);
> +	i = array_index_nospec(i, MAX_LEC_ITF);
>  	if (!dev_lec[i]) {
>  		int size;
>  

Why is this only for 4.19.y?  What is different in Linus's tree that
makes this not needed there?

thanks,

greg k-h

Re: net: atm: Spectre v1 fix introduced bug in bcb964012d1b in -stable

[Gustavo A. R. Silva]

On 5/20/19 7:40 AM, Pavel Machek wrote:
> 
> In lecd_attach, if arg is < 0, it was treated as 0. Spectre v1 fix
> changed that. Bug does not exist in mainline AFAICT.
> 

NACK

array_index_nospec() macro returns zero if *arg* is out of bounds.

In any case, if the "bug" does not exist in mainline, what should be
done is to port the code that fixed it in mainline to old kernels,
not randomly add a "fix" here and there because that means we are
breaking backward compatibility.

--
Gustavo

> Signed-off-by: Pavel Machek <pavel@denx.de>
> # for 4.19.y
> 
> diff --git a/net/atm/lec.c b/net/atm/lec.c
> index ad4f829193f0..ed279cd912f4 100644
> --- a/net/atm/lec.c
> +++ b/net/atm/lec.c
> @@ -731,7 +731,7 @@ static int lecd_attach(struct atm_vcc *vcc, int arg)
>  		i = arg;
>  	if (arg >= MAX_LEC_ITF)
>  		return -EINVAL;
> -	i = array_index_nospec(arg, MAX_LEC_ITF);
> +	i = array_index_nospec(i, MAX_LEC_ITF);
>  	if (!dev_lec[i]) {
>  		int size;
>  
> 

Re: net: atm: Spectre v1 fix introduced bug in bcb964012d1b in -stable

[Gustavo A. R. Silva]

On 5/20/19 9:00 AM, Greg KH wrote:
> On Mon, May 20, 2019 at 02:40:14PM +0200, Pavel Machek wrote:
>>
>> In lecd_attach, if arg is < 0, it was treated as 0. Spectre v1 fix
>> changed that. Bug does not exist in mainline AFAICT.
>>
>> Signed-off-by: Pavel Machek <pavel@denx.de>
>> # for 4.19.y
>>
>> diff --git a/net/atm/lec.c b/net/atm/lec.c
>> index ad4f829193f0..ed279cd912f4 100644
>> --- a/net/atm/lec.c
>> +++ b/net/atm/lec.c
>> @@ -731,7 +731,7 @@ static int lecd_attach(struct atm_vcc *vcc, int arg)
>>  		i = arg;
>>  	if (arg >= MAX_LEC_ITF)
>>  		return -EINVAL;
>> -	i = array_index_nospec(arg, MAX_LEC_ITF);
>> +	i = array_index_nospec(i, MAX_LEC_ITF);
>>  	if (!dev_lec[i]) {
>>  		int size;
>>  
> 
> Why is this only for 4.19.y?  What is different in Linus's tree that
> makes this not needed there?
> 

The only difference is this clean up:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fdd1a8103a6df50bdeacd8bb04c3f6976cb9ae41

As Dan says, the code works fine, but the *i* value wasn't being used
anymore, so that piece of code was a bit confusing.

Thanks
--
Gustavo

Re: net: atm: Spectre v1 fix introduced bug in bcb964012d1b in -stable

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]

On Mon 2019-05-20 16:00:07, Greg KH wrote:
> On Mon, May 20, 2019 at 02:40:14PM +0200, Pavel Machek wrote:
> > 
> > In lecd_attach, if arg is < 0, it was treated as 0. Spectre v1 fix
> > changed that. Bug does not exist in mainline AFAICT.
> > 
> > Signed-off-by: Pavel Machek <pavel@denx.de>
> > # for 4.19.y
> > 
> > diff --git a/net/atm/lec.c b/net/atm/lec.c
> > index ad4f829193f0..ed279cd912f4 100644
> > --- a/net/atm/lec.c
> > +++ b/net/atm/lec.c
> > @@ -731,7 +731,7 @@ static int lecd_attach(struct atm_vcc *vcc, int arg)
> >  		i = arg;
> >  	if (arg >= MAX_LEC_ITF)
> >  		return -EINVAL;
> > -	i = array_index_nospec(arg, MAX_LEC_ITF);
> > +	i = array_index_nospec(i, MAX_LEC_ITF);
> >  	if (!dev_lec[i]) {
> >  		int size;
> >  
> 
> Why is this only for 4.19.y?  What is different in Linus's tree that
> makes this not needed there?

Mainline sanitizes "arg", 4.19 copies "arg" into "i" then sanitizes
it. Take a look, it is local to the function.
								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: net: atm: Spectre v1 fix introduced bug in bcb964012d1b in -stable

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

On Mon 2019-05-20 09:15:50, Gustavo A. R. Silva wrote:
> 
> 
> On 5/20/19 7:40 AM, Pavel Machek wrote:
> > 
> > In lecd_attach, if arg is < 0, it was treated as 0. Spectre v1 fix
> > changed that. Bug does not exist in mainline AFAICT.
> > 
> 
> NACK
> 
> array_index_nospec() macro returns zero if *arg* is out of bounds.

No, it does not. Take a look:

#define array_index_nospec(index, size)
...
        (typeof(_i)) (_i & _mask);
})
	
								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: net: atm: Spectre v1 fix introduced bug in bcb964012d1b in -stable

[Gustavo A. R. Silva]

On 5/20/19 11:15 AM, Pavel Machek wrote:
> On Mon 2019-05-20 09:15:50, Gustavo A. R. Silva wrote:
>>
>>
>> On 5/20/19 7:40 AM, Pavel Machek wrote:
>>>
>>> In lecd_attach, if arg is < 0, it was treated as 0. Spectre v1 fix
>>> changed that. Bug does not exist in mainline AFAICT.
>>>
>>
>> NACK
>>
>> array_index_nospec() macro returns zero if *arg* is out of bounds.
> 
> No, it does not. Take a look:
> 
> #define array_index_nospec(index, size)
> ...
>         (typeof(_i)) (_i & _mask);
> })
> 	

Tell me more about that _mask.

Also, don't ignore the backward compatibility; otherwise you
would be forking your own kernel.

--
Gustavo

Re: net: atm: Spectre v1 fix introduced bug in bcb964012d1b in -stable

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

On Mon 2019-05-20 09:26:45, Gustavo A. R. Silva wrote:
> 
> 
> On 5/20/19 9:00 AM, Greg KH wrote:
> > On Mon, May 20, 2019 at 02:40:14PM +0200, Pavel Machek wrote:
> >>
> >> In lecd_attach, if arg is < 0, it was treated as 0. Spectre v1 fix
> >> changed that. Bug does not exist in mainline AFAICT.
> >>
> >> Signed-off-by: Pavel Machek <pavel@denx.de>
> >> # for 4.19.y
> >>
> >> diff --git a/net/atm/lec.c b/net/atm/lec.c
> >> index ad4f829193f0..ed279cd912f4 100644
> >> --- a/net/atm/lec.c
> >> +++ b/net/atm/lec.c
> >> @@ -731,7 +731,7 @@ static int lecd_attach(struct atm_vcc *vcc, int arg)
> >>  		i = arg;
> >>  	if (arg >= MAX_LEC_ITF)
> >>  		return -EINVAL;
> >> -	i = array_index_nospec(arg, MAX_LEC_ITF);
> >> +	i = array_index_nospec(i, MAX_LEC_ITF);
> >>  	if (!dev_lec[i]) {
> >>  		int size;
> >>  
> > 
> > Why is this only for 4.19.y?  What is different in Linus's tree that
> > makes this not needed there?
> > 
> 
> The only difference is this clean up:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fdd1a8103a6df50bdeacd8bb04c3f6976cb9ae41
> 
> As Dan says, the code works fine, but the *i* value wasn't being used
> anymore, so that piece of code was a bit confusing.

Yep, you are right, code managed to confused me: array_index_nospec
really returns 0 if it is out of bounds,

(typeof(_i)) (_i & _mask);

because _mask is always 0 or ~0.

Best regards,
								Pavel

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]