[bug report] cros_ec: Add vstore support

[Simon Glass]

Hi Dan,

On Tue, 25 Jul 2023 at 00:51, Dan Carpenter <dan.carpenter at 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