| Message ID | 42165d3f9dfc7abb54542d34a4e33ea8e83b101c.1648379172.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/rnbd: Fix the maximum clt_device_id value in init_dev() | expand |
On Sun, Mar 27, 2022 at 1:06 PM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > ida_alloc_range(..., min, max, ...) returns values from min to max, > inclusive. > > So, '1 << (MINORBITS - RNBD_PART_BITS)' is a valid value for ret, which is > then saved in 'dev->clt_device_id'. > > This value is used in rnbd_client_setup_device() and passed to > rnbd_clt_setup_gen_disk(). > There we have: > dev->gd->first_minor = idx << RNBD_PART_BITS > > So a possible value for 'gd->first_minor' is '1 << MINORBITS' > > This is an issue because: > rnbd_clt_setup_gen_disk() > --> add_disk(dev->gd) > --> device_add_disk(NULL, disk, NULL) > > And there we have: > ddev->devt = MKDEV(disk->major, disk->first_minor); > > So, should 'gd->first_minor' be '1 << MINORBITS', MKDEV() would overflow. > > Fixes: f7a7a5c228d4 ("block/rnbd: client: main functionality") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > #define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi)) > > This patch is completely speculative. > > I think that: > if (disk->first_minor + disk->minors > MINORMASK + 1) > return -EINVAL; > in device_add_disk() handles this corner case. > Anyway, if I'm correct, handling the error earlier can't hurt (at least I > guess so :)). > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> looks good to me, thx! Acked-by: Jack Wang <jinpu.wang@ionos.com> > --- > drivers/block/rnbd/rnbd-clt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c > index b66e8840b94b..db900c3786a3 100644 > --- a/drivers/block/rnbd/rnbd-clt.c > +++ b/drivers/block/rnbd/rnbd-clt.c > @@ -1454,7 +1454,7 @@ static struct rnbd_clt_dev *init_dev(struct rnbd_clt_session *sess, > goto out_alloc; > } > > - ret = ida_alloc_max(&index_ida, 1 << (MINORBITS - RNBD_PART_BITS), > + ret = ida_alloc_max(&index_ida, (1 << (MINORBITS - RNBD_PART_BITS)) - 1, > GFP_KERNEL); > if (ret < 0) { > pr_err("Failed to initialize device '%s' from session %s, allocating idr failed, err: %d\n", > -- > 2.32.0 >
diff --git a/drivers/block/rnbd/rnbd-clt.c b/drivers/block/rnbd/rnbd-clt.c index b66e8840b94b..db900c3786a3 100644 --- a/drivers/block/rnbd/rnbd-clt.c +++ b/drivers/block/rnbd/rnbd-clt.c @@ -1454,7 +1454,7 @@ static struct rnbd_clt_dev *init_dev(struct rnbd_clt_session *sess, goto out_alloc; } - ret = ida_alloc_max(&index_ida, 1 << (MINORBITS - RNBD_PART_BITS), + ret = ida_alloc_max(&index_ida, (1 << (MINORBITS - RNBD_PART_BITS)) - 1, GFP_KERNEL); if (ret < 0) { pr_err("Failed to initialize device '%s' from session %s, allocating idr failed, err: %d\n",
ida_alloc_range(..., min, max, ...) returns values from min to max, inclusive. So, '1 << (MINORBITS - RNBD_PART_BITS)' is a valid value for ret, which is then saved in 'dev->clt_device_id'. This value is used in rnbd_client_setup_device() and passed to rnbd_clt_setup_gen_disk(). There we have: dev->gd->first_minor = idx << RNBD_PART_BITS So a possible value for 'gd->first_minor' is '1 << MINORBITS' This is an issue because: rnbd_clt_setup_gen_disk() --> add_disk(dev->gd) --> device_add_disk(NULL, disk, NULL) And there we have: ddev->devt = MKDEV(disk->major, disk->first_minor); So, should 'gd->first_minor' be '1 << MINORBITS', MKDEV() would overflow. Fixes: f7a7a5c228d4 ("block/rnbd: client: main functionality") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- #define MKDEV(ma,mi) (((ma) << MINORBITS) | (mi)) This patch is completely speculative. I think that: if (disk->first_minor + disk->minors > MINORMASK + 1) return -EINVAL; in device_add_disk() handles this corner case. Anyway, if I'm correct, handling the error earlier can't hurt (at least I guess so :)). Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/block/rnbd/rnbd-clt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)