| Message ID | 20181129105519.mklhnmx34yjucb22@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ataflop: fix error handling in atari_floppy_init() | expand |
On 11/29/18 3:55 AM, Dan Carpenter wrote: > Smatch complains that there is an off by one if the allocation fails in: > > DMABuffer = atari_stram_alloc(BUFFER_SIZE+512, "ataflop"); > > In that situation, "i" would be point to one element beyond the end of > the unit[] array. > > There is a second bug because the error handling calls > blk_mq_free_tag_set(&unit[i].tag_set); regardless of whether > "disk->queue" is NULL or non-NULL. So if blk_mq_init_sq_queue() fails, > then that means unit[i].tag_set->tags is NULL and it leads to an Oops. > > It's easiest to call put_disk() before the goto to clean up the partial > iteration. Then the earlier unit[] elements are fully allocated so we > can remove the checks whether "disk->queue" is NULL and the code is > simpler. Applied, thanks. > I hope the Atari floppy disk users are appropriately grateful for all > the love and effort we put into their software... I'm sure that one person has you on the xmas shipping list.
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c index f88b4c26d422..313064b1005c 100644 --- a/drivers/block/ataflop.c +++ b/drivers/block/ataflop.c @@ -1982,6 +1982,7 @@ static int __init atari_floppy_init (void) &ataflop_mq_ops, 2, BLK_MQ_F_SHOULD_MERGE); if (IS_ERR(unit[i].disk->queue)) { + put_disk(unit[i].disk); ret = PTR_ERR(unit[i].disk->queue); unit[i].disk->queue = NULL; goto err; @@ -2033,18 +2034,13 @@ static int __init atari_floppy_init (void) return 0; err: - do { + while (--i >= 0) { struct gendisk *disk = unit[i].disk; - if (disk) { - if (disk->queue) { - blk_cleanup_queue(disk->queue); - disk->queue = NULL; - } - blk_mq_free_tag_set(&unit[i].tag_set); - put_disk(unit[i].disk); - } - } while (i--); + blk_cleanup_queue(disk->queue); + blk_mq_free_tag_set(&unit[i].tag_set); + put_disk(unit[i].disk); + } unregister_blkdev(FLOPPY_MAJOR, "fd"); return ret;
Smatch complains that there is an off by one if the allocation fails in: DMABuffer = atari_stram_alloc(BUFFER_SIZE+512, "ataflop"); In that situation, "i" would be point to one element beyond the end of the unit[] array. There is a second bug because the error handling calls blk_mq_free_tag_set(&unit[i].tag_set); regardless of whether "disk->queue" is NULL or non-NULL. So if blk_mq_init_sq_queue() fails, then that means unit[i].tag_set->tags is NULL and it leads to an Oops. It's easiest to call put_disk() before the goto to clean up the partial iteration. Then the earlier unit[] elements are fully allocated so we can remove the checks whether "disk->queue" is NULL and the code is simpler. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- I hope the Atari floppy disk users are appropriately grateful for all the love and effort we put into their software... drivers/block/ataflop.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)