Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 12 Jan 2021 22:42:59 +0900
From:   Takashi Sakamoto <o-takashi@sakamocchi.jp>
To:     Geert Uytterhoeven <geert+renesas@glider.be>
Cc:     Clemens Ladisch <clemens@ladisch.de>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, alsa-devel@alsa-project.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH/RFC 2/2] ALSA: firewire-tascam: Fix integer overflow in
 midi_port_work()
Message-ID: <20210112134259.GA44140@workstation>
Mail-Followup-To: Geert Uytterhoeven <geert+renesas@glider.be>,
        Clemens Ladisch <clemens@ladisch.de>,
        Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
References: <20210111130251.361335-1-geert+renesas@glider.be>
 <20210111130251.361335-3-geert+renesas@glider.be>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210111130251.361335-3-geert+renesas@glider.be>
Precedence: bulk

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@glider.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@sakamocchi.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