On Wed, May 10, 2017 at 07:20:50PM +0400, Dmitry Monakhov wrote:
> Currently if some one try to advance bvec beyond it's size we simply
> dump WARN_ONCE and continue to iterate beyond bvec array boundaries.
> This simply means that we endup dereferencing/corrupting random memory
> region.
>
> Sane reaction would be to propagate error back to calling context
> But bvec_iter_advance's calling context is not always good for error
> handling. For safity reason let truncate iterator size to zero which
> will break external iteration loop which prevent us from unpredictable
> memory range corruption. And even it caller ignores an error, it will
> corrupt it's own bvecs, not others.
>
> This patch does:
> - Return error back to caller with hope that it will react on this
> - Truncate iterator size
>
> Code was added long time ago here 4550dd6c, luckily no one hit it
> in real life :)
>
> changes since V1:
> - Replace BUG_ON with error logic.
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@xxxxxxxxxx>
> ---
> drivers/nvdimm/blk.c | 4 +++-
> drivers/nvdimm/btt.c | 4 +++-
> include/linux/bio.h | 8 ++++++--
> include/linux/bvec.h | 11 ++++++++---
> 4 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/nvdimm/blk.c b/drivers/nvdimm/blk.c
> index 0b49336..c82331b 100644
> --- a/drivers/nvdimm/blk.c
> +++ b/drivers/nvdimm/blk.c
> @@ -106,7 +106,9 @@ static int nd_blk_rw_integrity(struct nd_namespace_blk *nsblk,
>
> len -= cur_len;
> dev_offset += cur_len;
> - bvec_iter_advance(bip->bip_vec, &bip->bip_iter, cur_len);
> + err = bvec_iter_advance(bip->bip_vec, &bip->bip_iter, cur_len);
> + if (err)
> + return err;
> }
>
> return err;
> diff --git a/drivers/nvdimm/btt.c b/drivers/nvdimm/btt.c
> index 3d7a9fe..5a68681 100644
> --- a/drivers/nvdimm/btt.c
> +++ b/drivers/nvdimm/btt.c
> @@ -942,7 +942,9 @@ static int btt_rw_integrity(struct btt *btt, struct bio_integrity_payload *bip,
>
> len -= cur_len;
> meta_nsoff += cur_len;
> - bvec_iter_advance(bip->bip_vec, &bip->bip_iter, cur_len);
> + ret = bvec_iter_advance(bip->bip_vec, &bip->bip_iter, cur_len);
> + if (ret)
> + return ret;
> }
>
> return ret;
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 1b4ebb4..643ecba 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -168,8 +168,12 @@ static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
>
> if (bio_no_advance_iter(bio))
> iter->bi_size -= bytes;
> - else
> - bvec_iter_advance(bio->bi_io_vec, iter, bytes);
> + else {
> + int err;
> +
> + err = bvec_iter_advance(bio->bi_io_vec, iter, bytes);
> + /* TODO: It is reasonable to complete bio with error here. */
> + }
> }
>
> #define __bio_for_each_segment(bvl, bio, iter, start) \
> diff --git a/include/linux/bvec.h b/include/linux/bvec.h
> index 89b65b8..984a7a8 100644
> --- a/include/linux/bvec.h
> +++ b/include/linux/bvec.h
> @@ -22,6 +22,7 @@
>
> #include <linux/kernel.h>
> #include <linux/bug.h>
> +#include <linux/errno.h>
>
> /*
> * was unsigned short, but we might as well be ready for > 64kB I/O pages
> @@ -66,12 +67,15 @@ struct bvec_iter {
> .bv_offset = bvec_iter_offset((bvec), (iter)), \
> })
>
> -static inline void bvec_iter_advance(const struct bio_vec *bv,
> +static inline int bvec_iter_advance(const struct bio_vec *bv,
> struct bvec_iter *iter,
> unsigned bytes)
> {
> - WARN_ONCE(bytes > iter->bi_size,
> - "Attempted to advance past end of bvec iter\n");
> + if (WARN_ONCE(bytes > iter->bi_size,
> + "Attempted to advance past end of bvec iter\n")) {
> + iter->bi_size = 0;
> + return -EINVAL;
> + }
>
> while (bytes) {
> unsigned iter_len = bvec_iter_len(bv, *iter);
> @@ -86,6 +90,7 @@ static inline void bvec_iter_advance(const struct bio_vec *bv,
> iter->bi_idx++;
> }
> }
> + return 0;
> }
>
> #define for_each_bvec(bvl, bio_vec, iter, start) \
> --
> 2.9.3
>
Reviewed-by: Ming Lei <ming.lei@xxxxxxxxxx>
Thanks,
Ming
