| Message ID | 20230510173917.3073107-8-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: emu10k1: various improvements to the DSP-based mixer code | expand |
On Wed, 10 May 2023 19:39:10 +0200, Oswald Buddenhagen wrote: > > If these are hit, we've already trashed kernel memory by writing past > the end of the allocated buffer. There is no recovery from that. > > Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> > --- > v2: > - slightly more verbose description Again, this is NAK. First of all, if we really do care the overflow seriously, we should check at each increment instead of after breakage. It shouldn't be too difficult at all. Second, using BUG_ON() like this case is an overkill. It was clearly stated by Linus in the past a few times (although I can't find the source right now). thanks, Takashi > --- > sound/pci/emu10k1/emufx.c | 21 +++++---------------- > 1 file changed, 5 insertions(+), 16 deletions(-) > > diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c > index 2da1f9f1fb5a..5ae275d87c59 100644 > --- a/sound/pci/emu10k1/emufx.c > +++ b/sound/pci/emu10k1/emufx.c > @@ -1668,22 +1668,19 @@ A_OP(icode, &ptr, iMAC0, A_GPR(var), A_GPR(var), A_GPR(vol), A_EXTIN(input)) > * ok, set up done.. > */ > > - if (gpr > tmp) { > - snd_BUG(); > - err = -EIO; > - goto __err; > - } > + BUG_ON(gpr > tmp); > + BUG_ON(nctl > SND_EMU10K1_GPR_CONTROLS); > + > /* clear remaining instruction memory */ > while (ptr < 0x400) > A_OP(icode, &ptr, 0x0f, 0xc0, 0xc0, 0xcf, 0xc0); > > icode->gpr_add_control_count = nctl; > icode->gpr_add_controls = controls; > emu->support_tlv = 1; /* support TLV */ > err = snd_emu10k1_icode_poke(emu, icode, true); > emu->support_tlv = 0; /* clear again */ > > -__err: > kfree(controls); > __err_ctrls: > kfree(icode->gpr_map); > @@ -2272,16 +2269,8 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu) > } > > > - if (gpr > tmp) { > - snd_BUG(); > - err = -EIO; > - goto __err; > - } > - if (i > SND_EMU10K1_GPR_CONTROLS) { > - snd_BUG(); > - err = -EIO; > - goto __err; > - } > + BUG_ON(gpr > tmp); > + BUG_ON(i > SND_EMU10K1_GPR_CONTROLS); > > /* clear remaining instruction memory */ > while (ptr < 0x200) > -- > 2.40.0.152.g15d061e6df >
On Fri, May 12, 2023 at 09:15:17AM +0200, Takashi Iwai wrote: >On Wed, 10 May 2023 19:39:10 +0200, >Oswald Buddenhagen wrote: >> >> If these are hit, we've already trashed kernel memory by writing past >> the end of the allocated buffer. There is no recovery from that. >> >Again, this is NAK. >First of all, if we really do care the overflow >seriously, we should check at each increment instead of after >breakage. It shouldn't be too difficult at all. > not difficult, but pointless bloat. >Second, using BUG_ON() like this case is an overkill. It was clearly >stated by Linus in the past a few times (although I can't find the >source right now). > you seem to have an irrational aversion against assertions, maybe because linus likes to scream at people. relevant comments from linus were easy enough to find: https://yarchive.net/comp/linux/BUG.html https://lore.kernel.org/all/CA+55aFwyNTLuZgOWMTRuabWobF27ygskuxvFd-P0n-3UNT=0Og@mail.gmail.com/T/#u and there is also the documentation on BUG() itself. i don't see anything in either of these that would imply that my use of BUG_ON() is inappropriate. it catches a serious programming error, is easy to prove correct (the scope is a single function), and the only immediate effect is that it will crash the insmod process (though i would expect possible followup effects due to the kernel memory corruption, which is exactly why the assert is there). i have a hard time thinking of a *more* appropriate use for BUG(). regards
On Fri, 12 May 2023 11:25:23 +0200, Oswald Buddenhagen wrote: > > On Fri, May 12, 2023 at 09:15:17AM +0200, Takashi Iwai wrote: > > On Wed, 10 May 2023 19:39:10 +0200, > > Oswald Buddenhagen wrote: > >> > >> If these are hit, we've already trashed kernel memory by writing past > >> the end of the allocated buffer. There is no recovery from that. > >> > > Again, this is NAK. > > > First of all, if we really do care the overflow > > seriously, we should check at each increment instead of after > > breakage. It shouldn't be too difficult at all. > > > not difficult, but pointless bloat. > > > Second, using BUG_ON() like this case is an overkill. It was clearly > > stated by Linus in the past a few times (although I can't find the > > source right now). > > > you seem to have an irrational aversion against assertions, maybe > because linus likes to scream at people. Not because he's screaming but, it's because his opinion is correct regarding this. > relevant comments from linus were easy enough to find: > https://yarchive.net/comp/linux/BUG.html > https://lore.kernel.org/all/CA+55aFwyNTLuZgOWMTRuabWobF27ygskuxvFd-P0n-3UNT=0Og@mail.gmail.com/T/#u > > and there is also the documentation on BUG() itself. > > i don't see anything in either of these that would imply that my use > of BUG_ON() is inappropriate. it catches a serious programming error, > is easy to prove correct (the scope is a single function), and the > only immediate effect is that it will crash the insmod process (though > i would expect possible followup effects due to the kernel memory > corruption, which is exactly why the assert is there). i have a hard > time thinking of a *more* appropriate use for BUG(). I can't agree here at all. Sorry, but this is still NAK. The reason why BUG_ON() is bad is that it cannot allow debugging easily. It crashes and locks up, and you may not see what's going on. Do you want to catch and fix the bug? Then put the check at more proper pint that prevents the real corruption. the check is basically already too late -- it means that you might have already broken someone else's system. If the size matters, it can be a conditional build with CONFIG_SND_DEBUG, for example. But simply replacing and putting BUG_ON() makes little sense. Of course, it's just my opinion, but I won't change my mind about it. So I'm not going to discuss about this further and waste time. thanks, Takashi
diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c index 2da1f9f1fb5a..5ae275d87c59 100644 --- a/sound/pci/emu10k1/emufx.c +++ b/sound/pci/emu10k1/emufx.c @@ -1668,22 +1668,19 @@ A_OP(icode, &ptr, iMAC0, A_GPR(var), A_GPR(var), A_GPR(vol), A_EXTIN(input)) * ok, set up done.. */ - if (gpr > tmp) { - snd_BUG(); - err = -EIO; - goto __err; - } + BUG_ON(gpr > tmp); + BUG_ON(nctl > SND_EMU10K1_GPR_CONTROLS); + /* clear remaining instruction memory */ while (ptr < 0x400) A_OP(icode, &ptr, 0x0f, 0xc0, 0xc0, 0xcf, 0xc0); icode->gpr_add_control_count = nctl; icode->gpr_add_controls = controls; emu->support_tlv = 1; /* support TLV */ err = snd_emu10k1_icode_poke(emu, icode, true); emu->support_tlv = 0; /* clear again */ -__err: kfree(controls); __err_ctrls: kfree(icode->gpr_map); @@ -2272,16 +2269,8 @@ static int _snd_emu10k1_init_efx(struct snd_emu10k1 *emu) } - if (gpr > tmp) { - snd_BUG(); - err = -EIO; - goto __err; - } - if (i > SND_EMU10K1_GPR_CONTROLS) { - snd_BUG(); - err = -EIO; - goto __err; - } + BUG_ON(gpr > tmp); + BUG_ON(i > SND_EMU10K1_GPR_CONTROLS); /* clear remaining instruction memory */ while (ptr < 0x200)
If these are hit, we've already trashed kernel memory by writing past the end of the allocated buffer. There is no recovery from that. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- v2: - slightly more verbose description --- sound/pci/emu10k1/emufx.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-)