* [PATCH/RFC 0/2] ALSA: firewire: Fix integer overflows on 32-bit @ 2021-01-11 13:02 Geert Uytterhoeven 2021-01-11 13:02 ` [PATCH/RFC 1/2] ALSA: fireface: Fix integer overflow in transmit_midi_msg() Geert Uytterhoeven 2021-01-11 13:02 ` [PATCH/RFC 2/2] ALSA: firewire-tascam: Fix integer overflow in midi_port_work() Geert Uytterhoeven 0 siblings, 2 replies; 8+ messages in thread From: Geert Uytterhoeven @ 2021-01-11 13:02 UTC (permalink / raw) To: Clemens Ladisch, Takashi Sakamoto, Jaroslav Kysela, Takashi Iwai Cc: alsa-devel, linux-kernel, Geert Uytterhoeven Hi all, This patch series fixes two integer overflows on 32-bit platforms when in multiplications with the NSEC_PER_SEC constant, found by code inspection. They are marked "RFC", as I don't know the maximum transfer length of MIDI. Depending on this maximum length, a small tweak may be necessary. Thanks for your comments! Geert Uytterhoeven (2): ALSA: fireface: Fix integer overflow in transmit_midi_msg() ALSA: firewire-tascam: Fix integer overflow in midi_port_work() sound/firewire/fireface/ff-transaction.c | 2 +- sound/firewire/tascam/tascam-transaction.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- 2.25.1 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH/RFC 1/2] ALSA: fireface: Fix integer overflow in transmit_midi_msg() 2021-01-11 13:02 [PATCH/RFC 0/2] ALSA: firewire: Fix integer overflows on 32-bit Geert Uytterhoeven @ 2021-01-11 13:02 ` Geert Uytterhoeven 2021-01-12 13:53 ` Takashi Sakamoto 2021-01-12 13:58 ` Takashi Iwai 2021-01-11 13:02 ` [PATCH/RFC 2/2] ALSA: firewire-tascam: Fix integer overflow in midi_port_work() Geert Uytterhoeven 1 sibling, 2 replies; 8+ messages in thread From: Geert Uytterhoeven @ 2021-01-11 13:02 UTC (permalink / raw) To: Clemens Ladisch, Takashi Sakamoto, Jaroslav Kysela, Takashi Iwai Cc: alsa-devel, linux-kernel, Geert Uytterhoeven As snd_ff.rx_bytes[] is unsigned int, and NSEC_PER_SEC is 1000000000L, the second multiplication in ff->rx_bytes[port] * 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 ff->rx_bytes[port] <= 16777. Fixes: 19174295788de77d ("ALSA: fireface: add transaction support") 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/fireface/ff-transaction.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/firewire/fireface/ff-transaction.c b/sound/firewire/fireface/ff-transaction.c index 7f82762ccc8c80ba..ee7122c461d46f44 100644 --- a/sound/firewire/fireface/ff-transaction.c +++ b/sound/firewire/fireface/ff-transaction.c @@ -88,7 +88,7 @@ static void transmit_midi_msg(struct snd_ff *ff, unsigned int port) /* Set interval to next transaction. */ ff->next_ktime[port] = ktime_add_ns(ktime_get(), - ff->rx_bytes[port] * 8 * NSEC_PER_SEC / 31250); + ff->rx_bytes[port] * 8 * (NSEC_PER_SEC / 31250)); if (quad_count == 1) tcode = TCODE_WRITE_QUADLET_REQUEST; -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC 1/2] ALSA: fireface: Fix integer overflow in transmit_midi_msg() 2021-01-11 13:02 ` [PATCH/RFC 1/2] ALSA: fireface: Fix integer overflow in transmit_midi_msg() Geert Uytterhoeven @ 2021-01-12 13:53 ` Takashi Sakamoto 2021-01-12 13:58 ` Takashi Iwai 1 sibling, 0 replies; 8+ messages in thread From: Takashi Sakamoto @ 2021-01-12 13:53 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Clemens Ladisch, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel Hi, On Mon, Jan 11, 2021 at 02:02:50PM +0100, Geert Uytterhoeven wrote: > As snd_ff.rx_bytes[] is unsigned int, and NSEC_PER_SEC is 1000000000L, > the second multiplication in > > ff->rx_bytes[port] * 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 ff->rx_bytes[port] <= 16777. > > Fixes: 19174295788de77d ("ALSA: fireface: add transaction support") > 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/fireface/ff-transaction.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) The rx_bytes member has value for the length of byte messages to process. The range of value differs depending on Fireface protocol version. For former protocol, the value is equals to or less than SND_FF_MAXIMIM_MIDI_QUADS (= 9). For latter protocol, the value is equals to or less than 3. Anyway, the value should not be larger than 16777 and the calculation can be done without ULL suffix. Reviewd-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > diff --git a/sound/firewire/fireface/ff-transaction.c b/sound/firewire/fireface/ff-transaction.c > index 7f82762ccc8c80ba..ee7122c461d46f44 100644 > --- a/sound/firewire/fireface/ff-transaction.c > +++ b/sound/firewire/fireface/ff-transaction.c > @@ -88,7 +88,7 @@ static void transmit_midi_msg(struct snd_ff *ff, unsigned int port) > > /* Set interval to next transaction. */ > ff->next_ktime[port] = ktime_add_ns(ktime_get(), > - ff->rx_bytes[port] * 8 * NSEC_PER_SEC / 31250); > + ff->rx_bytes[port] * 8 * (NSEC_PER_SEC / 31250)); > > if (quad_count == 1) > tcode = TCODE_WRITE_QUADLET_REQUEST; > -- > 2.25.1 Thanks Takashi Sakamoto ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC 1/2] ALSA: fireface: Fix integer overflow in transmit_midi_msg() 2021-01-11 13:02 ` [PATCH/RFC 1/2] ALSA: fireface: Fix integer overflow in transmit_midi_msg() Geert Uytterhoeven 2021-01-12 13:53 ` Takashi Sakamoto @ 2021-01-12 13:58 ` Takashi Iwai 1 sibling, 0 replies; 8+ messages in thread From: Takashi Iwai @ 2021-01-12 13:58 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Clemens Ladisch, Takashi Sakamoto, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel On Mon, 11 Jan 2021 14:02:50 +0100, Geert Uytterhoeven wrote: > > As snd_ff.rx_bytes[] is unsigned int, and NSEC_PER_SEC is 1000000000L, > the second multiplication in > > ff->rx_bytes[port] * 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 ff->rx_bytes[port] <= 16777. > > Fixes: 19174295788de77d ("ALSA: fireface: add transaction support") > 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. Applied now. Thanks. Takashi ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH/RFC 2/2] ALSA: firewire-tascam: Fix integer overflow in midi_port_work() 2021-01-11 13:02 [PATCH/RFC 0/2] ALSA: firewire: Fix integer overflows on 32-bit Geert Uytterhoeven 2021-01-11 13:02 ` [PATCH/RFC 1/2] ALSA: fireface: Fix integer overflow in transmit_midi_msg() Geert Uytterhoeven @ 2021-01-11 13:02 ` Geert Uytterhoeven 2021-01-12 13:42 ` Takashi Sakamoto 2021-01-12 13:58 ` Takashi Iwai 1 sibling, 2 replies; 8+ messages in thread From: Geert Uytterhoeven @ 2021-01-11 13:02 UTC (permalink / raw) To: Clemens Ladisch, Takashi Sakamoto, Jaroslav Kysela, Takashi Iwai Cc: alsa-devel, linux-kernel, Geert Uytterhoeven 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(-) 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC 2/2] ALSA: firewire-tascam: Fix integer overflow in midi_port_work() 2021-01-11 13:02 ` [PATCH/RFC 2/2] ALSA: firewire-tascam: Fix integer overflow in midi_port_work() Geert Uytterhoeven @ 2021-01-12 13:42 ` Takashi Sakamoto 2021-01-12 13:55 ` Geert Uytterhoeven 2021-01-12 13:58 ` Takashi Iwai 1 sibling, 1 reply; 8+ messages in thread From: Takashi Sakamoto @ 2021-01-12 13:42 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Clemens Ladisch, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC 2/2] ALSA: firewire-tascam: Fix integer overflow in midi_port_work() 2021-01-12 13:42 ` Takashi Sakamoto @ 2021-01-12 13:55 ` Geert Uytterhoeven 0 siblings, 0 replies; 8+ messages in thread From: Geert Uytterhoeven @ 2021-01-12 13:55 UTC (permalink / raw) To: Takashi Sakamoto Cc: Clemens Ladisch, Jaroslav Kysela, Takashi Iwai, ALSA Development Mailing List, Linux Kernel Mailing List Hi Sakamoto-san, On Tue, Jan 12, 2021 at 2:43 PM Takashi Sakamoto <o-takashi@sakamocchi.jp> 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 <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. 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 <o-takashi@sakamocchi.jp> Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH/RFC 2/2] ALSA: firewire-tascam: Fix integer overflow in midi_port_work() 2021-01-11 13:02 ` [PATCH/RFC 2/2] ALSA: firewire-tascam: Fix integer overflow in midi_port_work() Geert Uytterhoeven 2021-01-12 13:42 ` Takashi Sakamoto @ 2021-01-12 13:58 ` Takashi Iwai 1 sibling, 0 replies; 8+ messages in thread From: Takashi Iwai @ 2021-01-12 13:58 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Clemens Ladisch, Takashi Sakamoto, Jaroslav Kysela, Takashi Iwai, alsa-devel, linux-kernel 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 <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. Applied now. Thanks. Takashi ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-12 14:00 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-11 13:02 [PATCH/RFC 0/2] ALSA: firewire: Fix integer overflows on 32-bit Geert Uytterhoeven 2021-01-11 13:02 ` [PATCH/RFC 1/2] ALSA: fireface: Fix integer overflow in transmit_midi_msg() Geert Uytterhoeven 2021-01-12 13:53 ` Takashi Sakamoto 2021-01-12 13:58 ` Takashi Iwai 2021-01-11 13:02 ` [PATCH/RFC 2/2] ALSA: firewire-tascam: Fix integer overflow in midi_port_work() Geert Uytterhoeven 2021-01-12 13:42 ` Takashi Sakamoto 2021-01-12 13:55 ` Geert Uytterhoeven 2021-01-12 13:58 ` Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).