[bug report] of: base: Introduce of_alias_get_alias_list() to check alias IDs

Hello Michal Simek,

The patch b1078c355d76: "of: base: Introduce
of_alias_get_alias_list() to check alias IDs" from Sep 20, 2018,
leads to the following Smatch static checker warning:

	drivers/of/base.c:2038 of_alias_get_alias_list()
	warn: passing negative bit value 's32min-(-2),0-s32max' to 'set_bit()'

drivers/of/base.c
    2028                 }
    2029 
    2030                 if (of_match_node(matches, app->np)) {
    2031                         pr_debug("%s: Allocated ID %d\n", __func__, app->id);
    2032 
    2033                         if (app->id >= nbits) {
    2034                                 pr_warn("%s: ID %d >= than bitmap field %d\n",
    2035                                         __func__, app->id, nbits);
    2036                                 ret = -EOVERFLOW;
    2037                         } else {
--> 2038                                 set_bit(app->id, bitmap);

Yes, there is a possibility for passing negative bits to set_bit().
But don't these bounds checks really belong in of_alias_add()?  Adding
invalid aliases seems like a risky thing.

    2039                         }
    2040                 }
    2041         }
    2042         mutex_unlock(&of_mutex);
    2043 
    2044         return ret;
    2045 }

regards,
dan carpenter

On Tue, Mar 15, 2022 at 10:09 AM Dan Carpenter [off-list ref] wrote:

Hello Michal Simek,

The patch b1078c355d76: "of: base: Introduce
of_alias_get_alias_list() to check alias IDs" from Sep 20, 2018,
leads to the following Smatch static checker warning:

        drivers/of/base.c:2038 of_alias_get_alias_list()
        warn: passing negative bit value 's32min-(-2),0-s32max' to 'set_bit()'

The 1 user of of_alias_get_alias_list() has since been refactored and
doesn't use of_alias_get_alias_list() anymore. So I'm just going to
revert b1078c355d76.

Rob

On 3/21/22 17:33, Rob Herring wrote:

On Tue, Mar 15, 2022 at 10:09 AM Dan Carpenter [off-list ref] wrote:

quoted

Hello Michal Simek,

The patch b1078c355d76: "of: base: Introduce
of_alias_get_alias_list() to check alias IDs" from Sep 20, 2018,
leads to the following Smatch static checker warning:

         drivers/of/base.c:2038 of_alias_get_alias_list()
         warn: passing negative bit value 's32min-(-2),0-s32max' to 'set_bit()'

The 1 user of of_alias_get_alias_list() has since been refactored and
doesn't use of_alias_get_alias_list() anymore. So I'm just going to
revert b1078c355d76.

yes. I am fine with it. We can get it back and fix it when it is again needed.

Thanks,
Michal