On Thu, Jul 27, 2023 at 1:21 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> I went through this pretty carefully. There is a small bug in the
> ORD_LIST_ELEMENTS case where the value_len is wrong. Otherwise, I
> complained a little about style nits... You can feel free to ignore
> everything except the ORD_LIST_ELEMENTS stuff.
>
> regards,
> dan carpenter
>
> drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
> 129 static int hp_populate_ordered_list_elements_from_package(union acpi_object *order_obj,
> 130 int order_obj_count,
> 131 int instance_id)
> 132 {
> 133 char *str_value = NULL;
>
> It would be better to make str_value local to the loop. Then we
> could delete all the str_value = NULL assignments and the exit_list:
> code at the end. At the end of the loop we could do something like:
>
> kfree(str_value);
> if (ret)
> return ret;
>
> 134 int value_len = 0;
> 135 int ret;
> 136 u32 size;
> 137 u32 int_value = 0;
>
> It confused me that it's called int_value when it's not an int. Just
> call it "u32 value = 0;".
The variable is named int_value when it is not an int because it
stores the value reported by ACPI_TYPE_INTEGER package.
The variable will be renamed to type_int_value;
>
> 138 int elem;
> 139 int reqs;
> 140 int eloc;
> 141 char *tmpstr = NULL;
> 142 char *part_tmp = NULL;
> 143 int tmp_len = 0;
> 144 char *part = NULL;
> 145 struct ordered_list_data *ordered_list_data = &bioscfg_drv.ordered_list_data[instance_id];
> 146
> 147 if (!order_obj)
> 148 return -EINVAL;
> 149
> 150 for (elem = 1, eloc = 1; elem < order_obj_count; elem++, eloc++) {
> 151 /* ONLY look at the first ORDERED_ELEM_CNT elements */
> 152 if (eloc == ORD_ELEM_CNT)
> 153 goto exit_list;
>
> We really expect to exit when eloc is ORD_ELEM_CNT. So I think this
> would be more clear as:
>
> for (elem = 1, eloc = 1; eloc < ORD_ELEM_CNT; elem++, eloc++) {
>
Done!
> I don't really know what eloc stands for... dst_idx?
>
> if (elem >= order_obj_count)
> return -EINVAL;
>
> obj = &order_obj[elem];
>
'eloc' stands for 'element location'. eloc helps keep track
conditions such when ORD_LIST_SIZE and/or PREREQUISITES_SiZE value is
zero.
> Let's move the "Check that both expected and read object type match"
> stuff from line 173 up to here.
>
> if (obj->type != expected_order_types[eloc]) {
> pr_err("Error expected type %d for elem %d, but got type %d instead\n",
> expected_order_types[eloc], elem, obj->type);
> return -EINVAL;
> }
>
> 154
> 155 switch (order_obj[elem].type) {
>
> switch(obj->type) {
>
> 156 case ACPI_TYPE_STRING:
> 157 if (elem != PREREQUISITES && elem != ORD_LIST_ELEMENTS) {
> 158 ret = hp_convert_hexstr_to_str(order_obj[elem].string.pointer,
> 159 order_obj[elem].string.length,
> 160 &str_value, &value_len);
> 161 if (ret)
> 162 continue;
>
> return ret;
>
> 163 }
> 164 break;
> 165 case ACPI_TYPE_INTEGER:
> 166 int_value = (u32)order_obj[elem].integer.value;
> 167 break;
> 168 default:
> 169 pr_warn("Unsupported object type [%d]\n", order_obj[elem].type);
> 170 continue;
>
> return -EINVAL;
We encountered during our testing that one or more packages could be
assigned the wrong package type or invalid data..
For this reason, it was decided to ignore any invalid data and
incorrect type package and allow the read process to continue.
>
> 171 }
> 172
> 173 /* Check that both expected and read object type match */
> 174 if (expected_order_types[eloc] != order_obj[elem].type) {
> 175 pr_err("Error expected type %d for elem %d, but got type %d instead\n",
> 176 expected_order_types[eloc], elem, order_obj[elem].type);
> 177 kfree(str_value);
> 178 return -EIO;
> 179 }
> 180
> 181 /* Assign appropriate element value to corresponding field*/
> 182 switch (eloc) {
> 183 case VALUE:
> 184 strscpy(ordered_list_data->current_value,
> 185 str_value, sizeof(ordered_list_data->current_value));
> 186 replace_char_str(ordered_list_data->current_value, COMMA_SEP, SEMICOLON_SEP);
> 187 break;
> 188 case PATH:
> 189 strscpy(ordered_list_data->common.path, str_value,
> 190 sizeof(ordered_list_data->common.path));
> 191 break;
> 192 case IS_READONLY:
> 193 ordered_list_data->common.is_readonly = int_value;
> 194 break;
> 195 case DISPLAY_IN_UI:
> 196 ordered_list_data->common.display_in_ui = int_value;
> 197 break;
> 198 case REQUIRES_PHYSICAL_PRESENCE:
> 199 ordered_list_data->common.requires_physical_presence = int_value;
> 200 break;
> 201 case SEQUENCE:
> 202 ordered_list_data->common.sequence = int_value;
> 203 break;
> 204 case PREREQUISITES_SIZE:
> 205 ordered_list_data->common.prerequisites_size = int_value;
> 206 if (int_value > MAX_PREREQUISITES_SIZE)
> 207 pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
>
> ret = -EINVAL;
> break;
>
We encountered during our testing that one or more packages could be
assigned the wrong package type or invalid data..
For this reason, it was decided to ignore any invalid data and
incorrect type package and allow the read process to continue.
> 208
> 209 /*
> 210 * This HACK is needed to keep the expected
> 211 * element list pointing to the right obj[elem].type
> 212 * when the size is zero. PREREQUISITES
> 213 * object is omitted by BIOS when the size is
> 214 * zero.
>
> It's not really a HACK.
Will change the term HACK to 'step'
>
> /*
> * If prerequisites_size is zero then there isn't a PREREQUISITES
> * object so skip that and jump to SECURITY_LEVEL.
> *
> */
>
>
> 215 */
> 216 if (int_value == 0)
> 217 eloc++;
> 218 break;
> 219 case PREREQUISITES:
> 220 size = min_t(u32, ordered_list_data->common.prerequisites_size,
> 221 MAX_PREREQUISITES_SIZE);
>
> If we returned when we hit invalid data then there is no need for this
> min_t().
>
> size = ordered_list_data->common.prerequisites_size;
>
> 222 for (reqs = 0; reqs < size; reqs++) {
> 223 ret = hp_convert_hexstr_to_str(order_obj[elem + reqs].string.pointer,
> 224 order_obj[elem + reqs].string.length,
> 225 &str_value, &value_len);
>
> This is fine but it might be nicer to do what ORD_LIST_ELEMENTS does
> and use tmpstr instead of str_value.
>
> 226
> 227 if (ret)
> 228 continue;
>
> break;
>
> 229
> 230 strscpy(ordered_list_data->common.prerequisites[reqs],
> 231 str_value,
> 232 sizeof(ordered_list_data->common.prerequisites[reqs]));
> 233
> 234 kfree(str_value);
> 235 str_value = NULL;
> 236 }
> 237 break;
> 238
> 239 case SECURITY_LEVEL:
> 240 ordered_list_data->common.security_level = int_value;
> 241 break;
> 242
> 243 case ORD_LIST_SIZE:
> 244 ordered_list_data->elements_size = int_value;
> 245 if (int_value > MAX_ELEMENTS_SIZE)
> 246 pr_warn("Ordered List size value exceeded the maximum number of elements supported or data may be malformed\n");
>
> ret = -EINVAL;
> break;
>
> 247 /*
> 248 * This HACK is needed to keep the expected
> 249 * element list pointing to the right obj[elem].type
> 250 * when the size is zero. ORD_LIST_ELEMENTS
> 251 * object is omitted by BIOS when the size is
> 252 * zero.
> 253 */
> 254 if (int_value == 0)
> 255 eloc++;
> 256 break;
> 257 case ORD_LIST_ELEMENTS:
> 258 size = ordered_list_data->elements_size;
>
> We don't use size anywhere so this can be deleted.
>
> 259
> 260 /*
> 261 * Ordered list data is stored in hex and comma separated format
> 262 * Convert the data and split it to show each element
> 263 */
> 264 ret = hp_convert_hexstr_to_str(str_value, value_len, &tmpstr, &tmp_len);
>
> The value_len here is wrong. We don't read value_len for ORD_LIST_ELEMENTS
> or PREREQUISITES so this value_len comes from the PATH object.
The size
>
> 265 if (ret)
> 266 goto exit_list;
> 267
> 268 part_tmp = tmpstr;
> 269 part = strsep(&part_tmp, COMMA_SEP);
> 270 if (!part)
> 271 strscpy(ordered_list_data->elements[0],
> 272 tmpstr,
> 273 sizeof(ordered_list_data->elements[0]));
> 274
> 275 for (elem = 1; elem < MAX_ELEMENTS_SIZE && part; elem++) {
>
> Please don't re-use the "elem" iterator for this inner loop.
Done!
>
> regards,
> dan carpenter
>
> 276 strscpy(ordered_list_data->elements[elem],
> 277 part,
> 278 sizeof(ordered_list_data->elements[elem]));
> 279 part = strsep(&part_tmp, SEMICOLON_SEP);
> 280 }
> 281
> 282 kfree(str_value);
> 283 str_value = NULL;
> 284 break;
> 285 default:
> 286 pr_warn("Invalid element: %d found in Ordered_List attribute or data may be malformed\n", elem);
> 287 break;
> 288 }
> 289 kfree(tmpstr);
> 290 tmpstr = NULL;
> 291 kfree(str_value);
> 292 str_value = NULL;
> 293 }
> 294
> 295 exit_list:
> 296 kfree(tmpstr);
> 297 kfree(str_value);
> 298 return 0;
> 299 }
>
>
