| Message ID | 20190309020252.27860-2-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ASoC: audio-graph-card: test memory allocation | expand |
Hi Pierre-Louis Thank you for your patch. > devm_kcalloc() return value needs to be tested > > Fixes: 17029e494edc6 ('ASoC: simple-card: add link_info') > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > sound/soc/generic/simple-card.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c > index 08df261024cf..bed4fe5721a2 100644 > --- a/sound/soc/generic/simple-card.c > +++ b/sound/soc/generic/simple-card.c > @@ -715,7 +715,7 @@ static int simple_probe(struct platform_device *pdev) > dai_link = devm_kcalloc(dev, li.link, sizeof(*dai_link), GFP_KERNEL); > dais = devm_kcalloc(dev, li.dais, sizeof(*dais), GFP_KERNEL); > cconf = devm_kcalloc(dev, li.conf, sizeof(*cconf), GFP_KERNEL); > - if (!dai_props || !dai_link || !dais) > + if (!dai_props || !dai_link || !dais || !cconf) > return -ENOMEM; It isn't mentioned on code, but, "li.conf" will be 0 if it was not DPCM case. This means cconf NULL might be happen. Best regards --- Kuninori Morimoto
On 3/10/19 7:20 PM, Kuninori Morimoto wrote: > > Hi Pierre-Louis > > Thank you for your patch. > >> devm_kcalloc() return value needs to be tested >> >> Fixes: 17029e494edc6 ('ASoC: simple-card: add link_info') >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> --- >> sound/soc/generic/simple-card.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c >> index 08df261024cf..bed4fe5721a2 100644 >> --- a/sound/soc/generic/simple-card.c >> +++ b/sound/soc/generic/simple-card.c >> @@ -715,7 +715,7 @@ static int simple_probe(struct platform_device *pdev) >> dai_link = devm_kcalloc(dev, li.link, sizeof(*dai_link), GFP_KERNEL); >> dais = devm_kcalloc(dev, li.dais, sizeof(*dais), GFP_KERNEL); >> cconf = devm_kcalloc(dev, li.conf, sizeof(*cconf), GFP_KERNEL); >> - if (!dai_props || !dai_link || !dais) >> + if (!dai_props || !dai_link || !dais || !cconf) >> return -ENOMEM; > > It isn't mentioned on code, but, "li.conf" will be 0 if it was not DPCM case. > This means cconf NULL might be happen. Humm, then don't call devm_kcalloc with a zero argument... And if you keep this code, if li.conf is not zero you still have a risk of memory allocation failure. the test should be + if (!dai_props || !dai_link || !dais || (!cconf && li.conf))
Hi Pierre > >> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c > >> index 08df261024cf..bed4fe5721a2 100644 > >> --- a/sound/soc/generic/simple-card.c > >> +++ b/sound/soc/generic/simple-card.c > >> @@ -715,7 +715,7 @@ static int simple_probe(struct platform_device *pdev) > >> dai_link = devm_kcalloc(dev, li.link, sizeof(*dai_link), GFP_KERNEL); > >> dais = devm_kcalloc(dev, li.dais, sizeof(*dais), GFP_KERNEL); > >> cconf = devm_kcalloc(dev, li.conf, sizeof(*cconf), GFP_KERNEL); > >> - if (!dai_props || !dai_link || !dais) > >> + if (!dai_props || !dai_link || !dais || !cconf) > >> return -ENOMEM; > > > > It isn't mentioned on code, but, "li.conf" will be 0 if it was not DPCM case. > > This means cconf NULL might be happen. > > > Humm, then don't call devm_kcalloc with a zero argument... > And if you keep this code, if li.conf is not zero you still have a > risk of memory allocation failure. the test should be > > + if (!dai_props || !dai_link || !dais || (!cconf && li.conf)) Yes, indeed. Thank you for your feedback Actually, I have few cleanup patches for simple-card/audio-graph for v5.2, then, your patch and it will be conflict. If you are OK, I can merge your patch on my patch-set with your Signed-off-by (or Reported-by). Is OK for you ? I will post it when -rc1 was released from Linus. Best regards --- Kuninori Morimoto
>>>> cconf = devm_kcalloc(dev, li.conf, sizeof(*cconf), GFP_KERNEL); >>>> - if (!dai_props || !dai_link || !dais) >>>> + if (!dai_props || !dai_link || !dais || !cconf) >>>> return -ENOMEM; >>> >>> It isn't mentioned on code, but, "li.conf" will be 0 if it was not DPCM case. >>> This means cconf NULL might be happen. >> >> >> Humm, then don't call devm_kcalloc with a zero argument... >> And if you keep this code, if li.conf is not zero you still have a >> risk of memory allocation failure. the test should be >> >> + if (!dai_props || !dai_link || !dais || (!cconf && li.conf)) > > Yes, indeed. > Thank you for your feedback > > Actually, I have few cleanup patches for simple-card/audio-graph > for v5.2, then, your patch and it will be conflict. > If you are OK, I can merge your patch on my patch-set with your > Signed-off-by (or Reported-by). > Is OK for you ? I will post it when -rc1 was released from Linus. yes, that's fine. Thanks!
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 08df261024cf..bed4fe5721a2 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -715,7 +715,7 @@ static int simple_probe(struct platform_device *pdev) dai_link = devm_kcalloc(dev, li.link, sizeof(*dai_link), GFP_KERNEL); dais = devm_kcalloc(dev, li.dais, sizeof(*dais), GFP_KERNEL); cconf = devm_kcalloc(dev, li.conf, sizeof(*cconf), GFP_KERNEL); - if (!dai_props || !dai_link || !dais) + if (!dai_props || !dai_link || !dais || !cconf) return -ENOMEM; /*
devm_kcalloc() return value needs to be tested Fixes: 17029e494edc6 ('ASoC: simple-card: add link_info') Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- sound/soc/generic/simple-card.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)