linux-kernel - Potentially broken error path in bio_map_user

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 3 Jan 2022 13:38:28 +0100
From:   Lukas Bulwahn <lukas.bulwahn@...il.com>
To:     Christoph Hellwig <hch@....de>
Cc:     Jens Axboe <axboe@...nel.dk>, linux-block@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        kernel-janitors <kernel-janitors@...r.kernel.org>
Subject: Potentially broken error path in bio_map_user_iov()

Dear Christoph,

in the function bio_map_user_iov() in ./block/blk-map.c, there is an
error branch for unlikely(offs & queue_dma_alignment(rq->q)), where
the supposingly expected error return value of the function is set,
i.e., ret = -EINVAL;. However, the variable ret is unconditionally
reset by the blk_rq_append_bio(...) call below, so that this 'ret =
-EINVAL;' assignment has no effect.

I am unsure which control flow you expect for this error case through
the bio_map_user_iov() function. Maybe you just want a patch like this
below to return the error code if it is at least once set?

diff --git a/block/blk-map.c b/block/blk-map.c
index 4526adde0156..4a3f6703f46f 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -235,7 +235,7 @@ static int bio_map_user_iov(struct request *rq,
struct iov_iter *iter,
 {
        unsigned int max_sectors = queue_max_hw_sectors(rq->q);
        struct bio *bio;
-       int ret;
+       int ret = 0;
        int j;

        if (!iov_iter_count(iter))
@@ -296,6 +296,9 @@ static int bio_map_user_iov(struct request *rq,
struct iov_iter *iter,
                        break;
        }

+       if (ret)
+               goto out_unmap;
+
        ret = blk_rq_append_bio(rq, bio);
        if (ret)
                goto out_unmap;

I know too little about this function and its intent to create a
proper patch, though.

I also looked at the previous versions, but the error code of this
error branch was really never effectively returned before either (as
far as I grasped the earlier versions of this function). So, this
error path probably never worked as intended.


Best regards,

Lukas