Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sd: sd_ops: Data Race in card_ioctl #72368

Closed
clamattia opened this issue May 6, 2024 · 4 comments · Fixed by #72757
Closed

sd: sd_ops: Data Race in card_ioctl #72368

clamattia opened this issue May 6, 2024 · 4 comments · Fixed by #72757
Assignees
Labels
area: Disk Access bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@clamattia
Copy link

The function card_ioctl in subsys/sd/sd_ops.c calls sdmmc_wait_ready, which calls sdhc_card_busy without taking the card->lock mutex. This leads to a data-race on the lower layers (for example spi).

Problematic line: https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/sd/sd_ops.c#L790

Suggested fix:

-   switch (cmd) {
-   case DISK_IOCTL_GET_SECTOR_COUNT:
-       (*(uint32_t *)buf) = card->block_count;
-       break;
-   case DISK_IOCTL_GET_SECTOR_SIZE:
-   case DISK_IOCTL_GET_ERASE_BLOCK_SZ:
-       (*(uint32_t *)buf) = card->block_size;
-       break;
-   case DISK_IOCTL_CTRL_SYNC:
-       /* Ensure card is not busy with data write.
-        * Note that SD stack does not support enabling caching, so
-        * cache flush is not required here
-        */
-       return sdmmc_wait_ready(card);
-   default:
-       return -ENOTSUP;
-   }
-   return 0;
+   int ret = 0;
+   ret = k_mutex_lock(&card->lock, K_MSEC(CONFIG_SD_DATA_TIMEOUT));
+   if (ret) {
+       LOG_WRN("Could not get SD card mutex");
+       return -EBUSY;
+   }
+   switch (cmd) {
+   case DISK_IOCTL_GET_SECTOR_COUNT:
+   (*(uint32_t *)buf) = card->block_count;
+       break;
+   case DISK_IOCTL_GET_SECTOR_SIZE:
+   case DISK_IOCTL_GET_ERASE_BLOCK_SZ:
+       (*(uint32_t *)buf) = card->block_size;
+       break;
+   case DISK_IOCTL_CTRL_SYNC:
+       /* Ensure card is not busy with data write.
+        * Note that SD stack does not support enabling caching, so
+        * cache flush is not required here
+        */
+       ret = sdmmc_wait_ready(card);
+       break;
+   default:
+       ret = -ENOTSUP;
+       break;
+   }
+   k_mutex_unlock(&card->lock);
+   return ret;

To reproduce: two threads concurrently accessing the sd-card. At least one of them issues ioclts. For example filesystem.

Expected behavior: No data race. With a data race, anything is possible. Of the possible scenarios, crashes are the least problematic.

Severly impacts all applications with concurrent access to the sd-card, when at least one of them uses ioctl.

Similar mutex problem in same file: #72287

Let me know, if you need additional information.

@clamattia clamattia added the bug The issue is a bug, or the PR is fixing a bug label May 6, 2024
@nashif nashif changed the title subsystem: sd: sd_ops: Data Race in card_ioctl sd: sd_ops: Data Race in card_ioctl May 7, 2024
@nashif nashif added area: Disk Access priority: medium Medium impact/importance bug labels May 7, 2024
@andyross
Copy link
Contributor

andyross commented May 7, 2024

Nice catch. But FWIW: "Suggested fixes" are usually best expressed as pull requests. :)

@clamattia
Copy link
Author

Nice catch. But FWIW: "Suggested fixes" are usually best expressed as pull requests. :)

Good point. Will try to do so next time 👍

danieldegrasse added a commit to nxp-upstream/zephyr that referenced this issue May 14, 2024
Take card lock when running IOCTL command, to avoid race conditions that
could occur within the lower SDHC transfer implementations (as these
will be called by sdmmc_wait_ready)

Fixes zephyrproject-rtos#72368

Signed-off-by: Daniel DeGrasse <[email protected]>
@danieldegrasse
Copy link
Collaborator

Good point. Will try to do so next time 👍

Thanks for catching this, I have created a pull request: #72757, with your suggested change

@clamattia
Copy link
Author

Thank you @danieldegrasse

danieldegrasse added a commit to nxp-upstream/zephyr that referenced this issue May 14, 2024
Take card lock when running IOCTL command, to avoid race conditions that
could occur within the lower SDHC transfer implementations (as these
will be called by sdmmc_wait_ready)

Fixes zephyrproject-rtos#72368

Signed-off-by: Daniel DeGrasse <[email protected]>
nashif pushed a commit that referenced this issue May 16, 2024
Take card lock when running IOCTL command, to avoid race conditions that
could occur within the lower SDHC transfer implementations (as these
will be called by sdmmc_wait_ready)

Fixes #72368

Signed-off-by: Daniel DeGrasse <[email protected]>
mariopaja pushed a commit to mariopaja/zephyr that referenced this issue May 26, 2024
Take card lock when running IOCTL command, to avoid race conditions that
could occur within the lower SDHC transfer implementations (as these
will be called by sdmmc_wait_ready)

Fixes zephyrproject-rtos#72368

Signed-off-by: Daniel DeGrasse <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Disk Access bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants