Re: [bug report] cros_ec: Add vstore support

Hi Dan,

On Tue, 25 Jul 2023 at 00:51, Dan Carpenter dan.carpenter@linaro.org wrote:

Hello Simon Glass,

The patch 10f746591fba: "cros_ec: Add vstore support" from Jan 16, 2021 (linux-next), leads to the following Smatch static checker warning:

drivers/misc/cros_ec_sandbox.c:543 process_cmd() error: buffer overflow 'ec->slot' 4 <= 31 drivers/misc/cros_ec_sandbox.c:556 process_cmd() error: buffer overflow 'ec->slot' 4 <= 31

drivers/misc/cros_ec_sandbox.c 521 len = sizeof(*resp); 522 break; 523 } 524 case EC_CMD_VSTORE_INFO: { 525 struct ec_response_vstore_info *resp = resp_data; 526 int i; 527 528 resp->slot_count = VSTORE_SLOT_COUNT;

There are two related defines. VSTORE_SLOT_COUNT (4) is the number of elements in ec->slot[].

529                 resp->slot_locked = 0;
530                 for (i = 0; i < VSTORE_SLOT_COUNT; i++) {
531                         if (ec->slot[i].locked)
532                                 resp->slot_locked |= 1 << i;
533                 }
534                 len = sizeof(*resp);
535                 break;
536         };
537         case EC_CMD_VSTORE_WRITE: {
538                 const struct ec_params_vstore_write *req = req_data;
539                 struct vstore_slot *slot;
540
541                 if (req->slot >= EC_VSTORE_SLOT_MAX)
542                         return -EINVAL;

--> 543 slot = &ec->slot[req->slot];

But here the check is for EC_VSTORE_SLOT_MAX (32) so Smatch thinks that 32 is more than 4 so this is an out of bounds. Should the limit be smaller or the array larger?

The limit should be EC_VSTORE_SLOT_COUNT since we don't support the max number of slots in the emulator.

So this should check for EC_VSTORE_SLOT_COUNT instead of EC_VSTORE_SLOT_MAX.

544                 slot->locked = true;
545                 memcpy(slot->data, req->data, EC_VSTORE_SLOT_SIZE);
546                 len = 0;
547                 break;
548         }
549         case EC_CMD_VSTORE_READ: {
550                 const struct ec_params_vstore_read *req = req_data;
551                 struct ec_response_vstore_read *resp = resp_data;
552                 struct vstore_slot *slot;
553
554                 if (req->slot >= EC_VSTORE_SLOT_MAX)
555                         return -EINVAL;
556                 slot = &ec->slot[req->slot];

Same.

Same here.

557                 memcpy(resp->data, slot->data, EC_VSTORE_SLOT_SIZE);
558                 len = sizeof(*resp);
559                 break;
560         }
561         case EC_CMD_PWM_GET_DUTY: {
562                 const struct ec_params_pwm_get_duty *req = req_data;

Regards, Simon