| Message ID | 20240416114509.198069-1-r.smirnov@omp.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] media: tuners: tda18271: fix error code handling in tda18271_attach() | expand |
On 4/16/24 2:45 PM, Roman Smirnov wrote: > tda18271_attach() uses the hybrid_tuner_request_state() macro. > It may return the error code -ENOMEM, but the function handle > the value 0 instead. > > Found by Linux Verification Center (linuxtesting.org) with Svace. > > Fixes: b9302fa7ed97 ("media: tuners: fix error return code of hybrid_tuner_request_state()") > Signed-off-by: Roman Smirnov <r.smirnov@omp.ru> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey
Hello Roman, On Tue, 16. Apr 14:45, Roman Smirnov wrote: > tda18271_attach() uses the hybrid_tuner_request_state() macro. > It may return the error code -ENOMEM, but the function handle > the value 0 instead. Maybe hybrid_tuner_request_state macro declaration should be fixed to generate zero in case of a memory allocation failure? At least it has a comment stating the following * 0 - no instances, indicates an error - kzalloc must have failed And supposedly a number of drivers implemented the error handling based on this assumption. The drivers mentioned in this series are not the only ones susceptible to the problem. Grepping through "hybrid_tuner_request_state" calls also gives out tda9887, xc2028, r820t and others. > > Found by Linux Verification Center (linuxtesting.org) with Svace. > > Fixes: b9302fa7ed97 ("media: tuners: fix error return code of hybrid_tuner_request_state()") > Signed-off-by: Roman Smirnov <r.smirnov@omp.ru> > --- > drivers/media/tuners/tda18271-fe.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/tuners/tda18271-fe.c b/drivers/media/tuners/tda18271-fe.c > index a7e721baaa99..23432210f06a 100644 > --- a/drivers/media/tuners/tda18271-fe.c > +++ b/drivers/media/tuners/tda18271-fe.c > @@ -1255,7 +1255,7 @@ struct dvb_frontend *tda18271_attach(struct dvb_frontend *fe, u8 addr, > hybrid_tuner_instance_list, > i2c, addr, "tda18271"); > switch (instance) { > - case 0: > + case -ENOMEM: > goto fail; > case 1: > /* new tuner instance */ > -- > 2.34.1 >
On Wed, 24. Apr 21:06, Fedor Pchelkin wrote: > Hello Roman, > > On Tue, 16. Apr 14:45, Roman Smirnov wrote: > > tda18271_attach() uses the hybrid_tuner_request_state() macro. > > It may return the error code -ENOMEM, but the function handle > > the value 0 instead. > > Maybe hybrid_tuner_request_state macro declaration should be fixed to > generate zero in case of a memory allocation failure? > > At least it has a comment stating the following > * 0 - no instances, indicates an error - kzalloc must have failed > > And supposedly a number of drivers implemented the error handling based on > this assumption. > > The drivers mentioned in this series are not the only ones susceptible to > the problem. Grepping through "hybrid_tuner_request_state" calls also gives > out tda9887, xc2028, r820t and others. > > > > > Found by Linux Verification Center (linuxtesting.org) with Svace. > > > > Fixes: b9302fa7ed97 ("media: tuners: fix error return code of hybrid_tuner_request_state()") Looking more thoroughly, I think commit b9302fa7ed97 ("media: tuners: fix error return code of hybrid_tuner_request_state()") should be reverted because it just contradicts with the return values contract which is stated in the comment for the macro and which is followed by all the existing drivers. __ret should be assigned 0 in error case as was before the commit. > > Signed-off-by: Roman Smirnov <r.smirnov@omp.ru> > > --- > > drivers/media/tuners/tda18271-fe.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/tuners/tda18271-fe.c b/drivers/media/tuners/tda18271-fe.c > > index a7e721baaa99..23432210f06a 100644 > > --- a/drivers/media/tuners/tda18271-fe.c > > +++ b/drivers/media/tuners/tda18271-fe.c > > @@ -1255,7 +1255,7 @@ struct dvb_frontend *tda18271_attach(struct dvb_frontend *fe, u8 addr, > > hybrid_tuner_instance_list, > > i2c, addr, "tda18271"); > > switch (instance) { > > - case 0: > > + case -ENOMEM: > > goto fail; > > case 1: > > /* new tuner instance */ > > -- > > 2.34.1 > >
diff --git a/drivers/media/tuners/tda18271-fe.c b/drivers/media/tuners/tda18271-fe.c index a7e721baaa99..23432210f06a 100644 --- a/drivers/media/tuners/tda18271-fe.c +++ b/drivers/media/tuners/tda18271-fe.c @@ -1255,7 +1255,7 @@ struct dvb_frontend *tda18271_attach(struct dvb_frontend *fe, u8 addr, hybrid_tuner_instance_list, i2c, addr, "tda18271"); switch (instance) { - case 0: + case -ENOMEM: goto fail; case 1: /* new tuner instance */
tda18271_attach() uses the hybrid_tuner_request_state() macro. It may return the error code -ENOMEM, but the function handle the value 0 instead. Found by Linux Verification Center (linuxtesting.org) with Svace. Fixes: b9302fa7ed97 ("media: tuners: fix error return code of hybrid_tuner_request_state()") Signed-off-by: Roman Smirnov <r.smirnov@omp.ru> --- drivers/media/tuners/tda18271-fe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)