| Message ID | 20210111130251.361335-2-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e7c22eeaff8565d9a8374f320238c251ca31480b |
| Headers | show |
| Series | ALSA: firewire: Fix integer overflows on 32-bit | expand |
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
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
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;
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(-)