If iov_offset is non-zero, then we need to consider iov_offset in length
calculation, otherwise we might pass smaller IOs such as 512 bytes
with 256 bytes offset.

Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
---
 lib/iov_iter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 8c7fdb7d8c8f..aa80fb094004 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -820,7 +820,7 @@ static bool iov_iter_aligned_bvec(const struct iov_iter *i, unsigned addr_mask,
 	size_t size = i->count;
 
 	do {
-		size_t len = bvec->bv_len;
+		size_t len = bvec->bv_len - skip;
 
 		if (len > size)
 			len = size;

base-commit: 834a4a689699090a406d1662b03affa8b155d025
-- 
2.43.0

On 4/15/25 12:14 PM, Nitesh Shetty wrote:
> If iov_offset is non-zero, then we need to consider iov_offset in length
> calculation, otherwise we might pass smaller IOs such as 512 bytes
> with 256 bytes offset.

As Andrew points out, would be nicer with a (much) better commit
message. Your current reply has a lot of the stuff that should go in
there, including a Fixes tag. With that done:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

On Tue, 15 Apr 2025 23:44:19 +0530 Nitesh Shetty <nj.shetty@samsung.com> wrote:

> If iov_offset is non-zero, then we need to consider iov_offset in length
> calculation, otherwise we might pass smaller IOs such as 512 bytes
> with 256 bytes offset.
> 

Please describe the userspace-visible effects of this flaw, if any?

> --- a/lib/iov_iter.c
> +++ b/lib/iov_iter.c
> @@ -820,7 +820,7 @@ static bool iov_iter_aligned_bvec(const struct iov_iter *i, unsigned addr_mask,
>  	size_t size = i->count;
>  
>  	do {
> -		size_t len = bvec->bv_len;
> +		size_t len = bvec->bv_len - skip;
>  
>  		if (len > size)
>  			len = size;
>

On Thu, Apr 17, 2025 at 5:25 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue, 15 Apr 2025 23:44:19 +0530 Nitesh Shetty <nj.shetty@samsung.com> wrote:
>
> > If iov_offset is non-zero, then we need to consider iov_offset in length
> > calculation, otherwise we might pass smaller IOs such as 512 bytes
> > with 256 bytes offset.
> >
>
> Please describe the userspace-visible effects of this flaw, if any?
>
a.
At present we see different behaviour based on whether the device is
512 byte LBA formatted or 4K byte LBA formatted.

Here is the scenario, where this case is possible, when using io-uring
fixed buffer IO with 512 LBA device we can get,
i->count = 512, i->iov_offset = 3584, bvec->bv_offset = 256,
bvec->bv_len = 3840, len_mask = 511

In this effectively, the first 256 bytes are in the current page,
next 256 bytes are in the second page.
This should fail the IO, but here we pass.
(For a similar setting with i->count=4096, this check fails).

To reproduce the test from user space, we can use a liburing application [1]
with patch[2], on a 512 LBA format block device.

b.
I went through file history and looks like this is a regression,
previously the fix was present, but commit
2263639f96f24a121ec9f037981b81daf5a8d60a introduced this issue it seems.

Thanks,
Nitesh Shetty

[1] https://github.com/axboe/liburing/blob/master/test/fixed-seg.c
[2]
diff --git a/test/fixed-seg.c b/test/fixed-seg.c
index 4154abb..e7c461a 100644
--- a/test/fixed-seg.c
+++ b/test/fixed-seg.c
@@ -64,7 +64,7 @@ static int test(struct io_uring *ring, int fd, int vec_off)
                return T_EXIT_FAIL;
        }

-       ret = read_it(ring, fd, 4096, vec_off);
+       ret = read_it(ring, fd, 512, vec_off + 512*7);
        if (ret) {
                fprintf(stderr, "4096 0 failed\n");
                return T_EXIT_FAIL;
@@ -158,7 +158,7 @@ int main(int argc, char *argv[])
                goto err;
        }

-       ret = test(&ring, fd, 0);
+       ret = test(&ring, fd, 256);
        if (ret) {
                fprintf(stderr, "test 0 failed\n");
                goto err;