[Bug Report]Sound: sound/core/hwdep.c undefined result by left shifting 1 by 31

[Bug Report]Sound: sound/core/hwdep.c undefined result by left shifting 1 by 31

[Changming Liu]

Hi Jaroslav, Takashi:
Greetings, I'm a first year PhD student who is interested in using UBSan for linux. 
After some experiments, I found that in sound/core/hwdep.c function snd_hwdep_dsp_load 
there might be an undefined behavior that might cause unexpected result.

More specifically, in this function,info was fetched from user space and,
info.index was checked if it's greater than or equal to 32.
If not then it's used as number of left shift bits to string literal 1.

The problem is, since string literal 1 is by default signed int, so 1 << 31 cannot be represented as a valid integer and
 the result might be undefined across different platforms. So I guess change 1 to 1U might help?

Due to the lack of knowledge of the interaction between this module and others, I'm not able to assess if 
this is critical or worth fixing. I'd appreciate if for your comment on this bug. This can help me understand UB a lot!

Looking forward to your response.

Best,
Changming Liu

Re: [Bug Report]Sound: sound/core/hwdep.c undefined result by left shifting 1 by 31

[Takashi Iwai]

On Fri, 22 May 2020 01:32:00 +0200,
Changming Liu wrote:
> 
> Hi Jaroslav, Takashi:
> Greetings, I'm a first year PhD student who is interested in using UBSan for linux. 
> After some experiments, I found that in sound/core/hwdep.c function snd_hwdep_dsp_load 
> there might be an undefined behavior that might cause unexpected result.
> 
> More specifically, in this function,info was fetched from user space and,
> info.index was checked if it's greater than or equal to 32.
> If not then it's used as number of left shift bits to string literal 1.
> 
> The problem is, since string literal 1 is by default signed int, so 1 << 31 cannot be represented as a valid integer and
>  the result might be undefined across different platforms. So I guess change 1 to 1U might help?

Yes, that should work.

> Due to the lack of knowledge of the interaction between this module and others, I'm not able to assess if 
> this is critical or worth fixing. I'd appreciate if for your comment on this bug. This can help me understand UB a lot!

I don't think this is any serious problem.  It's a bit flag that just
indicates the status and the flag itself has any influence on the
behavior of the rest in kernel.  And, the hwdep DSP load feature
itself is used very rarely, only by a couple of drivers.  So it's
pretty much never hit.

That said, we can simply fix the issue at any time, and it's "just to
be sure".  Could you submit a fix patch in the proper format?


thanks,

Takashi

RE: [Bug Report]Sound: sound/core/hwdep.c undefined result by left shifting 1 by 31

[Changming Liu]

> From: Takashi Iwai <tiwai@suse.de>
> Sent: Monday, May 25, 2020 5:57 AM
> To: Changming Liu <liu.changm@northeastern.edu>
> Cc: perex@perex.cz; tiwai@suse.com; alsa-devel@alsa-project.org; Lu, Long
> <l.lu@northeastern.edu>; yaohway@gmail.com
> Subject: Re: [Bug Report]Sound: sound/core/hwdep.c undefined result by left
> shifting 1 by 31
> 
> On Fri, 22 May 2020 01:32:00 +0200,
> Changming Liu wrote:
> >
> > Hi Jaroslav, Takashi:
> > Greetings, I'm a first year PhD student who is interested in using UBSan for
> linux.
> > After some experiments, I found that in sound/core/hwdep.c function
> snd_hwdep_dsp_load
> > there might be an undefined behavior that might cause unexpected result.
> >
> > More specifically, in this function,info was fetched from user space and,
> > info.index was checked if it's greater than or equal to 32.
> > If not then it's used as number of left shift bits to string literal 1.
> >
> > The problem is, since string literal 1 is by default signed int, so 1 << 31 cannot
> be represented as a valid integer and
> >  the result might be undefined across different platforms. So I guess change 1
> to 1U might help?
> 
> Yes, that should work.
> 
> > Due to the lack of knowledge of the interaction between this module and
> others, I'm not able to assess if
> > this is critical or worth fixing. I'd appreciate if for your comment on this bug.
> This can help me understand UB a lot!
> 
> I don't think this is any serious problem.  It's a bit flag that just
> indicates the status and the flag itself has any influence on the
> behavior of the rest in kernel.  And, the hwdep DSP load feature
> itself is used very rarely, only by a couple of drivers.  So it's
> pretty much never hit.
 
Thank you for this valuable information which vividly demonstrates 
how inter-module interaction affects the severity of an undefined value.
It's valuable.

> That said, we can simply fix the issue at any time, and it's "just to
> be sure".  Could you submit a fix patch in the proper format?
> 
Sure, it's a honor, I'll submit a patch fixing this in a separated email.

Thanks,
Changming