ALSA: emu10k1: shift wrapping bug in snd_emu10k1_ptr_read()
Subsystems (2)
PATCH 1/1 ALSA: emu10k1: shift wrapping bug in snd_emu10k1_ptr_read()
Static analysis says "size" is a number 0-63. So we really want to be doing the shift as a u64 and not a int type. Presumably "size" is never actually more than 31 otherwise the shift wrap would have been detected in testing. The "mask" is a u32 so we only care about the bottom 32 bits which also implies that "size" is less than 32.
This code pre-dates git. I haven't tested this change, it's to fix a static analysis warning. I can't think that shift wrapping is the correct behavior so presumably this change is harmless but it definitely changes how the code works when size is larger than 32.
sound/pci/emu10k1/io.c +2 -2
@@ -46,7 +46,7 @@ unsigned int snd_emu10k1_ptr_read(struct snd_emu10k1 * emu, unsigned int reg, un size = (reg >>24 ) &0x3f ; offset = (reg >>16 ) &0x1f ; - mask = ((1 << size) -1 ) << offset; + mask = ((1ULL << size) -1 ) << offset;spin_lock_irqsave (&emu->emu_lock, flags);outl (regptr, emu->port +PTR );
@@ -81,7 +81,7 @@ void snd_emu10k1_ptr_write(struct snd_emu10k1 *emu, unsigned int reg, unsigned i size = (reg >>24 ) &0x3f ; offset = (reg >>16 ) &0x1f ; - mask = ((1 << size) -1 ) << offset; + mask = ((1ULL << size) -1 ) << offset; data = (data << offset) & mask;spin_lock_irqsave (&emu->emu_lock, flags);
Quoted 11 lines · Dan Carpenter
Static analysis says "size" is a number 0-63. So we really want to be doing the shift as a u64 and not a int type. Presumably "size" is never actually more than 31 otherwise the shift wrap would have been detected in testing. The "mask" is a u32 so we only care about the bottom 32 bits which also implies that "size" is less than 32.
This code pre-dates git. I haven't tested this change, it's to fix a static analysis warning. I can't think that shift wrapping is the correct behavior so presumably this change is harmless but it definitely changes how the code works when size is larger than 32.
IIRC, this is a kind of encoded register, so it should be never overflow. And looking through the current code, this encoded register is nowhere used. It was used only in the past in the prototype driver. If my quick analysis is correct, the relevant code may be even dropped.
For now, I'm inclined to leave the stuff as is. We may add a WARN_ON() if this really matters. But it's a stone age driver code, and this particular part has never been a problem, so WARN_ON() will be most likely nothing but a memory waste.
thanks,
Takashi
Quoted 26 lines
Signed-off-by Dan Carpenter <[email protected]>sound/pci/emu10k1/io.c +2
-2@@ -46,7 +46,7 @@ unsigned int snd_emu10k1_ptr_read(struct snd_emu10k1 * emu, unsigned int reg, un size = (reg >>24 ) &0x3f ; offset = (reg >>16 ) &0x1f ; - mask = ((1 << size) -1 ) << offset; + mask = ((1ULL << size) -1 ) << offset;spin_lock_irqsave (&emu->emu_lock, flags);outl (regptr, emu->port +PTR );@@ -81,7 +81,7 @@ void snd_emu10k1_ptr_write(struct snd_emu10k1 *emu, unsigned int reg, unsigned i size = (reg >>24 ) &0x3f ; offset = (reg >>16 ) &0x1f ; - mask = ((1 << size) -1 ) << offset; + mask = ((1ULL << size) -1 ) << offset; data = (data << offset) & mask;spin_lock_irqsave (&emu->emu_lock, flags);