[bug report] ASoC: mediatek: mt8188: support etdm in platform driver

[Trevor Wu (吳文良)]

Hi Dan,

Thank you for catching that.
I didn't notice that snprintf() never returns a negative value in
kernel code.
It is possible that I misunderstood the coverity error message, but I
can't find the original coverity scan result.

If I just remove the dead code, coverity will report some errors in the
future.
As a result, I prefer to keep the line and add another check "ret >=
sizeof(prop)", so that we can avoid the problem reported by coverity
and Smatch.


Thanks,
Trevor


On Wed, 2023-01-25 at 15:13 +0300, Dan Carpenter wrote:
> Hello Trevor Wu,
> 
> The patch 2babb4777489: "ASoC: mediatek: mt8188: support etdm in
> platform driver" from Jan 16, 2023, leads to the following Smatch
> static checker warning:
> 
> 	sound/soc/mediatek/mt8188/mt8188-dai-etdm.c:2487
> mt8188_dai_etdm_parse_of()
> 	warn: 'ret' returned from snprintf() might be larger than 48
> 
> sound/soc/mediatek/mt8188/mt8188-dai-etdm.c
>     2455 static void mt8188_dai_etdm_parse_of(struct mtk_base_afe
> *afe)
>     2456 {
>     2457         const struct device_node *of_node = afe->dev-
> >of_node;
>     2458         struct mt8188_afe_private *afe_priv = afe-
> >platform_priv;
>     2459         struct mtk_dai_etdm_priv *etdm_data;
>     2460         char prop[48];
>     2461         u8 disable_chn[MT8188_ETDM_MAX_CHANNELS];
>     2462         int max_chn = MT8188_ETDM_MAX_CHANNELS;
>     2463         unsigned int sync_id;
>     2464         u32 sel;
>     2465         int ret;
>     2466         int dai_id;
>     2467         int i, j;
>     2468         struct {
>     2469                 const char *name;
>     2470                 const unsigned int sync_id;
>     2471         } of_afe_etdms[MT8188_AFE_IO_ETDM_NUM] = {
>     2472                 {"etdm-in1", ETDM_SYNC_FROM_IN1},
>     2473                 {"etdm-in2", ETDM_SYNC_FROM_IN2},
>     2474                 {"etdm-out1", ETDM_SYNC_FROM_OUT1},
>     2475                 {"etdm-out2", ETDM_SYNC_FROM_OUT2},
>     2476                 {"etdm-out3", ETDM_SYNC_FROM_OUT3},
>     2477         };
>     2478 
>     2479         for (i = 0; i < MT8188_AFE_IO_ETDM_NUM; i++) {
>     2480                 dai_id = ETDM_TO_DAI_ID(i);
>     2481                 etdm_data = afe_priv->dai_priv[dai_id];
>     2482 
>     2483                 ret = snprintf(prop, sizeof(prop),
>     2484                                "mediatek,%s-multi-pin-mode",
>     2485                                of_afe_etdms[i].name);
>     2486                 if (ret < 0) {
> --> 2487                         dev_err(afe->dev, "%s snprintf
> err=%d\n",
>     2488                                 __func__, ret);
> 
> 
> I guess the reason behind this warning is that we checked something
> but
> we didn't check if "ret >= sizeof(prop)" as one would normally
> suspect.
> 
> In user space snprintf() can return negatives but in the kernel it
> never
> does.  Changing that in the future is impossible because it would
> introduce a ton of security issues, so it never will.
> 
> I guess I suggest just removing the dead code.
> 
>     2489                         return;
>     2490                 }
>     2491                 etdm_data->data_mode =
> of_property_read_bool(of_node, prop);
>     2492 
>     2493                 ret = snprintf(prop, sizeof(prop),
>     2494                                "mediatek,%s-cowork-source",
>     2495                                of_afe_etdms[i].name);
>     2496                 if (ret < 0) {
> 
> Same
> 
>     2497                         dev_err(afe->dev, "%s snprintf
> err=%d\n",
>     2498                                 __func__, ret);
>     2499                         return;
>     2500                 }
>     2501                 ret = of_property_read_u32(of_node, prop,
> &sel);
>     2502                 if (ret == 0) {
>     2503                         if (sel >= MT8188_AFE_IO_ETDM_NUM) {
>     2504                                 dev_err(afe->dev, "%s
> invalid id=%d\n",
>     2505                                         __func__, sel);
>     2506                                 etdm_data->cowork_source_id
> = COWORK_ETDM_NONE;
>     2507                         } else {
>     2508                                 sync_id =
> of_afe_etdms[sel].sync_id;
>     2509                                 etdm_data->cowork_source_id
> =
>     2510                                         sync_to_dai_id(sync_
> id);
>     2511                         }
>     2512                 } else {
>     2513                         etdm_data->cowork_source_id =
> COWORK_ETDM_NONE;
>     2514                 }
>     2515         }
>     2516 
>     2517         /* etdm in only */
>     2518         for (i = 0; i < 2; i++) {
>     2519                 ret = snprintf(prop, sizeof(prop),
>     2520                                "mediatek,%s-chn-disabled",
>     2521                                of_afe_etdms[i].name);
>     2522                 if (ret < 0) {
> 
> Same.
> 
>     2523                         dev_err(afe->dev, "%s snprintf
> err=%d\n",
>     2524                                 __func__, ret);
>     2525                         return;
>     2526                 }
>     2527                 ret =
> of_property_read_variable_u8_array(of_node, prop,
>     2528                                                          dis
> able_chn,
>     2529                                                          1,
> max_chn);
>     2530                 if (ret < 0)
>     2531                         continue;
>     2532 
>     2533                 for (j = 0; j < ret; j++) {
>     2534                         if (disable_chn[j] >=
> MT8188_ETDM_MAX_CHANNELS)
>     2535                                 dev_err(afe->dev, "%s [%d]
> invalid chn %u\n",
>     2536                                         __func__, j,
> disable_chn[j]);
>     2537                         else
>     2538                                 etdm_data-
> >in_disable_ch[disable_chn[j]] = true;
>     2539                 }
>     2540         }
>     2541         mt8188_etdm_update_sync_info(afe);
>     2542 }
> 
> regards,
> dan carpenter