[PATCH 0/1] block: fix potential infinite loop in the negative branch in __submit_bio_noacct_mq()

[PATCH 0/1] block: fix potential infinite loop in the negative branch in __submit_bio_noacct_mq()

[Sergei Shtepa]

Hi all.

It seems to me that the __submit_bio_noacct_mq() function incorrectly
processes the return code of the blk_crypto_bio_prep() function.

If the blk_crypto_bio_prep() function returns false, it means that
the processing of the bio request was completed with an error and
further processing of the request is unnecessary.

But in the code, in case of an error when executing the
blk_crypto_bio_prep() function, an attempt is made to repeat the
execution of this function. This can lead to an infinite loop.
In addition, since the function __blk_crypto_bio_prep calls bio_endio(),
it is likely to access the freed data or access the null pointer.

At the same time, the implementation of the negative branch of the
blk_crypto_bio_prep() function implemented correctly in the
__submit_bio_noacct() and __submit_bio() functions.

Sergei Shtepa (1):
  block: fix potential infinite loop in the negative branch in
    __submit_bio_noacct_mq()

 block/blk-core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

-- 
2.20.1

[PATCH 1/1] block: fix potential infinite loop in the negative branch in __submit_bio_noacct_mq()

[Sergei Shtepa]

When the blk_crypto_bio_prep() function returns false, the processing
of the bio request must end. Repeated access to blk_crypto_bio_prep()
for this same bio may lead to access to already released data, since in
this case the bio_endio() function was already called for bio.

The changes allow to leave the processing of the failed bio and
go to the next one from the bio_list.

The error can only occur when using inline encryption on
request-based blk-mq devices and something went wrong in the
__blk_crypto_bio_prep().

Signed-off-by: Sergei Shtepa <sergei.shtepa@veeam.com>
---
 block/blk-core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index fc60ff208497..825df223b01d 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1005,13 +1005,12 @@ static blk_qc_t __submit_bio_noacct_mq(struct bio *bio)
 		if (unlikely(bio_queue_enter(bio) != 0))
 			continue;
 
-		if (!blk_crypto_bio_prep(&bio)) {
+		if (blk_crypto_bio_prep(&bio))
+			ret = blk_mq_submit_bio(bio);
+		else {
 			blk_queue_exit(disk->queue);
 			ret = BLK_QC_T_NONE;
-			continue;
 		}
-
-		ret = blk_mq_submit_bio(bio);
 	} while ((bio = bio_list_pop(&bio_list[0])));
 
 	current->bio_list = NULL;
-- 
2.20.1

Re: [PATCH 1/1] block: fix potential infinite loop in the negative branch in __submit_bio_noacct_mq()

[Christoph Hellwig]

On Tue, Mar 23, 2021 at 02:48:36PM +0300, Sergei Shtepa wrote:
> When the blk_crypto_bio_prep() function returns false, the processing
> of the bio request must end. Repeated access to blk_crypto_bio_prep()
> for this same bio may lead to access to already released data, since in
> this case the bio_endio() function was already called for bio.
> 
> The changes allow to leave the processing of the failed bio and
> go to the next one from the bio_list.
> 
> The error can only occur when using inline encryption on
> request-based blk-mq devices and something went wrong in the
> __blk_crypto_bio_prep().

A continue in a do { } while statement evaluates the while condition,
so your patch is a no-op.

Re: [PATCH 1/1] block: fix potential infinite loop in the negative branch in __submit_bio_noacct_mq()

[Sergei Shtepa]

The 03/24/2021 11:18, Christoph Hellwig wrote:
> On Tue, Mar 23, 2021 at 02:48:36PM +0300, Sergei Shtepa wrote:
> > When the blk_crypto_bio_prep() function returns false, the processing
> > of the bio request must end. Repeated access to blk_crypto_bio_prep()
> > for this same bio may lead to access to already released data, since in
> > this case the bio_endio() function was already called for bio.
> > 
> > The changes allow to leave the processing of the failed bio and
> > go to the next one from the bio_list.
> > 
> > The error can only occur when using inline encryption on
> > request-based blk-mq devices and something went wrong in the
> > __blk_crypto_bio_prep().
> 
> A continue in a do { } while statement evaluates the while condition,
> so your patch is a no-op.

Thank you Christoph!
Shame on my bald head.
I apologize and will be more attentive in the future.
-- 
Sergei Shtepa
Veeam Software developer.