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

FreeRTOS F_DRIVER bug in block driver read/write #1

Closed
aamcampbell opened this issue Oct 28, 2015 · 1 comment
Closed

FreeRTOS F_DRIVER bug in block driver read/write #1

aamcampbell opened this issue Oct 28, 2015 · 1 comment
Labels

Comments

@aamcampbell
Copy link

The FreeRTOS F_DRIVER block device read and write functions use the wrong variable(s) for the sector index.

From os/freertos/services/osbdev.c, line 478 (DiskRead()):

    for(ulSectorIdx = 0U; ulSectorIdx < ulSectorCount; ulSectorIdx++)
    {
        iErr = pDriver->readsector(pDriver, &pbBuffer[ulSectorIdx * ulSectorSize],
                                   CAST_ULONG(ullSectorStart + ulSectorCount));
    /*...*/
    }

Line 534 contains similar logic for the DiskWrite() function.

The F_DRIVER readsector and writesector methods can only handle one sector at a time, so they are called from within a for loop so that multiple sectors can be transferred by the RedOsBDevRead and Write methods. The loop iterator is ulSectorIdx, which is correctly used to find the write place in the given buffer. However, the loop iterator is ignored when determining the sector number pass to readsector and writesector. Instead, (ullSectorStart + ulSectorCount) is passed as the sector number.

The values of ullSectorStart and ulSectorCount are not modified within the loop. For multi-sector transfers, this means that the same sector will be read or written repetitively instead of the expected number of sectors being transferred to or from the disk. Furthermore, the sector being accessed is always one beyond the end of the requested transfer, which affects single-sector transfers as well. For example, on single-sector transfers (ulSectorCount = 1), the desired sector is at ullSectorStart, but ullSectorStart + 1 is used as the sector number.

The FreeRTOS BDEV_F_DRIVER implementations of DiskRead and DiskWrite (os/freertos/services/osbdev.c) are affected by this bug. Other block device service implementations are not affected.

This issue report is for documentation purposes only; the bug has already been fixed.

@aamcampbell
Copy link
Author

The correct value to pass to readsector() and writesector() is (ullSectorStart + ulSectorIdx). This change has been made to both the DiskRead() and DiskWrite() methods. See commit 9104a7e (Trunk Build 665).

@aamcampbell aamcampbell added resolved and removed bug labels Dec 6, 2016
danielrlewis added a commit that referenced this issue Mar 8, 2023
Checking for read permissions (via R_OK) in InodeMustBeSearchableDir()
is either redundant or incorrect.  Remove it.

Assume dirfd is a file descriptor for a directory whose permission bits
are 0111 (i.e., search permissions only).  Consider the following API
calls:

1. red_openat(dirfd, "..", RED_O_RDONLY)
2. red_openat(dirfd, "./..", RED_O_RDONLY)
3. red_openat(dirfd, ".", RED_O_RDONLY)

Both #1 and #2 should succeed (assuming dirfd's parent directory has
read permissions).  However, InodeMustBeSearchableDir() would cause
both #1 and #2 to fail, since it will check whether dirfd has read
permissions, even though those aren't required.

Case #3 should fail, and does fail, but InodeMustBeSearchableDir() does
not need to be the place where this failure originates, because the
check for read permissions will occur later, in FildesOpen().  The R_OK
check in InodeMustBeSearchableDir() is redundant.

See also commit 80c2c79, the origin of the relevant code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant