lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 12 Jan 2021 22:42:59 +0900
From:   Takashi Sakamoto <o-takashi@...amocchi.jp>
To:     Geert Uytterhoeven <geert+renesas@...der.be>
Cc:     Clemens Ladisch <clemens@...isch.de>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH/RFC 2/2] ALSA: firewire-tascam: Fix integer overflow in
 midi_port_work()

Hi,

On Mon, Jan 11, 2021 at 02:02:51PM +0100, Geert Uytterhoeven wrote:
> As snd_fw_async_midi_port.consume_bytes is unsigned int, and
> NSEC_PER_SEC is 1000000000L, the second multiplication in
> 
>     port->consume_bytes * 8 * NSEC_PER_SEC / 31250
> 
> always overflows on 32-bit platforms, truncating the result.  Fix this
> by precalculating "NSEC_PER_SEC / 31250", which is an integer constant.
> 
> Note that this assumes port->consume_bytes <= 16777.
> 
> Fixes: 531f471834227d03 ("ALSA: firewire-lib/firewire-tascam: localize async midi port")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
> ---
> Compile-tested only.
> 
> I don't know the maximum transfer length of MIDI, but given it's an old
> standard, I guess it's rather small.  If it is larger than 16777, the
> constant "8" should be replaced by "8ULL", to force 64-bit arithmetic.
> ---
>  sound/firewire/tascam/tascam-transaction.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
 
Indeed. The calculation brings integer overflow of 32 bit storage. Thanks
for your care and proposal of the patch. I agree with the intension of
patch, however I have a nitpicking that the consume_bytes member is
defined as 'int', not 'unsigned int' in your commit comment.

The member has value returned from the call of 'fill_message()'[1] for the
length of byte messages in buffer to process, or for error code. The
error code is checked immediately. The range of value is equal to
or less than 3 when reaching the calculation, thus it should be less than
16777.

Regardless of the type of 'int' or 'unsigned int', this patch can fix
the issued problem. Feel free to add my tag when you post second version
with comment fix.

Reviewed-by: Takashi Sakamoto <o-takashi@...amocchi.jp>

> diff --git a/sound/firewire/tascam/tascam-transaction.c b/sound/firewire/tascam/tascam-transaction.c
> index 90288b4b46379526..a073cece4a7d5e3a 100644
> --- a/sound/firewire/tascam/tascam-transaction.c
> +++ b/sound/firewire/tascam/tascam-transaction.c
> @@ -209,7 +209,7 @@ static void midi_port_work(struct work_struct *work)
>  
>  	/* Set interval to next transaction. */
>  	port->next_ktime = ktime_add_ns(ktime_get(),
> -				port->consume_bytes * 8 * NSEC_PER_SEC / 31250);
> +			port->consume_bytes * 8 * (NSEC_PER_SEC / 31250));
>  
>  	/* Start this transaction. */
>  	port->idling = false;
> -- 
> 2.25.1

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/sound/firewire/tascam/tascam-transaction.c#n197

Thanks

Takashi Sakamoto

Powered by blists - more mailing lists