| Message ID | c3b3d66c-1fb1-8bb9-cf00-674fd4e20afb@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 06, 2017 at 09:46:52PM +0200, SF Markus Elfring wrote: > @@ -336,21 +331,24 @@ static int snd_card_ymfpci_probe(struct pci_dev *pci, > legacy_ctrl &= ~YMFPCI_LEGACY_FMEN; > pci_write_config_word(pci, PCIR_DSXG_LEGACY, legacy_ctrl); > } else if ((err = snd_opl3_hwdep_new(opl3, 0, 1, NULL)) < 0) { > - snd_card_free(card); ^^^^^^^^^^^^^^^^^^^ > dev_err(card->dev, "cannot create opl3 hwdep\n"); ^^^^^^^^^ > - return err; > + goto free_card; Heh. I was worried that some of these re-orderings would introduce bugs but actually this one fixes a use after free. regards, dan carpenter
>> @@ -336,21 +331,24 @@ static int snd_card_ymfpci_probe(struct pci_dev *pci, >> legacy_ctrl &= ~YMFPCI_LEGACY_FMEN; >> pci_write_config_word(pci, PCIR_DSXG_LEGACY, legacy_ctrl); >> } else if ((err = snd_opl3_hwdep_new(opl3, 0, 1, NULL)) < 0) { >> - snd_card_free(card); > ^^^^^^^^^^^^^^^^^^^ >> dev_err(card->dev, "cannot create opl3 hwdep\n"); > ^^^^^^^^^ >> - return err; >> + goto free_card; > > Heh. I was worried that some of these re-orderings would introduce bugs > but actually this one fixes a use after free. Thanks for your constructive feedback. Does it mean that a special tag should be added to a commit message? Regards, Markus
On Thu, 07 Sep 2017 09:41:39 +0200, SF Markus Elfring wrote: > > >> @@ -336,21 +331,24 @@ static int snd_card_ymfpci_probe(struct pci_dev *pci, > >> legacy_ctrl &= ~YMFPCI_LEGACY_FMEN; > >> pci_write_config_word(pci, PCIR_DSXG_LEGACY, legacy_ctrl); > >> } else if ((err = snd_opl3_hwdep_new(opl3, 0, 1, NULL)) < 0) { > >> - snd_card_free(card); > > ^^^^^^^^^^^^^^^^^^^ > >> dev_err(card->dev, "cannot create opl3 hwdep\n"); > > ^^^^^^^^^ > >> - return err; > >> + goto free_card; > > > > Heh. I was worried that some of these re-orderings would introduce bugs > > but actually this one fixes a use after free. > > Thanks for your constructive feedback. > > Does it mean that a special tag should be added to a commit message? No need for resend, I'll add some more notes at merging. thanks, Takashi
On Wed, 06 Sep 2017 21:46:52 +0200, SF Markus Elfring wrote: > > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Wed, 6 Sep 2017 20:45:11 +0200 > > * Add a jump target so that a bit of exception handling can be better > reused at the end of this function. > > This issue was detected by using the Coccinelle software. > > * The script "checkpatch.pl" pointed information out like the following. > > ERROR: do not use assignment in if condition > > Thus fix a few source code places. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> Applied now. Thanks. Takashi
diff --git a/sound/pci/ymfpci/ymfpci.c b/sound/pci/ymfpci/ymfpci.c index 4faf3e1ed06a..eafdee384059 100644 --- a/sound/pci/ymfpci/ymfpci.c +++ b/sound/pci/ymfpci/ymfpci.c @@ -268,10 +268,9 @@ static int snd_card_ymfpci_probe(struct pci_dev *pci, if ((err = snd_ymfpci_create(card, pci, old_legacy_ctrl, &chip)) < 0) { - snd_card_free(card); release_and_free_resource(mpu_res); release_and_free_resource(fm_res); - return err; + goto free_card; } chip->fm_res = fm_res; chip->mpu_res = mpu_res; @@ -283,35 +282,31 @@ static int snd_card_ymfpci_probe(struct pci_dev *pci, card->shortname, chip->reg_area_phys, chip->irq); - if ((err = snd_ymfpci_pcm(chip, 0)) < 0) { - snd_card_free(card); - return err; - } - if ((err = snd_ymfpci_pcm_spdif(chip, 1)) < 0) { - snd_card_free(card); - return err; - } + err = snd_ymfpci_pcm(chip, 0); + if (err < 0) + goto free_card; + + err = snd_ymfpci_pcm_spdif(chip, 1); + if (err < 0) + goto free_card; + err = snd_ymfpci_mixer(chip, rear_switch[dev]); - if (err < 0) { - snd_card_free(card); - return err; - } + if (err < 0) + goto free_card; + if (chip->ac97->ext_id & AC97_EI_SDAC) { err = snd_ymfpci_pcm_4ch(chip, 2); - if (err < 0) { - snd_card_free(card); - return err; - } + if (err < 0) + goto free_card; + err = snd_ymfpci_pcm2(chip, 3); - if (err < 0) { - snd_card_free(card); - return err; - } - } - if ((err = snd_ymfpci_timer(chip, 0)) < 0) { - snd_card_free(card); - return err; + if (err < 0) + goto free_card; } + err = snd_ymfpci_timer(chip, 0); + if (err < 0) + goto free_card; + if (chip->mpu_res) { if ((err = snd_mpu401_uart_new(card, 0, MPU401_HW_YMFPCI, mpu_port[dev], @@ -336,21 +331,24 @@ static int snd_card_ymfpci_probe(struct pci_dev *pci, legacy_ctrl &= ~YMFPCI_LEGACY_FMEN; pci_write_config_word(pci, PCIR_DSXG_LEGACY, legacy_ctrl); } else if ((err = snd_opl3_hwdep_new(opl3, 0, 1, NULL)) < 0) { - snd_card_free(card); dev_err(card->dev, "cannot create opl3 hwdep\n"); - return err; + goto free_card; } } snd_ymfpci_create_gameport(chip, dev, legacy_ctrl, legacy_ctrl2); - if ((err = snd_card_register(card)) < 0) { - snd_card_free(card); - return err; - } + err = snd_card_register(card); + if (err < 0) + goto free_card; + pci_set_drvdata(pci, card); dev++; return 0; + +free_card: + snd_card_free(card); + return err; } static void snd_card_ymfpci_remove(struct pci_dev *pci)