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 <[email protected]>
---
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(-)
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
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 <[email protected]>
> ---
> 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 <[email protected]>
> 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
Hi Sakamoto-san,
On Tue, Jan 12, 2021 at 2:43 PM Takashi Sakamoto
<[email protected]> wrote:
> 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 <[email protected]>
> > ---
> > 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.
Thanks, you're right.
Note that port->consume_bytes being signed halves the limit to
8388 bytes, which is of course still met.
> 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 <[email protected]>
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, 11 Jan 2021 14:02:51 +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 <[email protected]>
> ---
> 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.
Applied now. Thanks.
Takashi