linux-kernel - RE: [PATCH] block: create ioctl to discard-or-zeroout a range of blocks

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-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 11 Nov 2015 05:30:07 +0000
From:	"Seymour, Shane M" <shane.seymour@....com>
To:	"Darrick J. Wong" <darrick.wong@...cle.com>,
	Jens Axboe <axboe@...nel.dk>,
	Christoph Hellwig <hch@...radead.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
	Jeff Layton <jlayton@...chiereds.net>,
	"J. Bruce Fields" <bfields@...ldses.org>,
	"martin.petersen@...cle.com" <martin.petersen@...cle.com>
Subject: RE: [PATCH] block: create ioctl to discard-or-zeroout a range of
 blocks

A quick question about this part of the patch:

> +	uint64_t end = start + len - 1;

> +	if (end >= i_size_read(bdev->bd_inode))
 		return -EINVAL;

> +	/* Invalidate the page cache, including dirty pages */
> +	mapping = bdev->bd_inode->i_mapping;
> +	truncate_inode_pages_range(mapping, start, end);

blk_ioctl_zeroout accepts unsigned values for start and end (uint64_t) but loff_t types are turned from i_size_read() and passed as the 2nd and 3rd values to truncate_inode_pages_range() and loff_t is a signed value. It should be possible to pass in some values would overflow the calculation of end causing the test on the value of end and the result of i_size_read to pass but then end up passing a large unsigned value for in start that would be implicitly converted to signed in truncate_inode_pages_range. I was wondering if you'd tested passing in data that would cause sign conversion issues when passed into truncate_inode_pages_range (does it handle it gracefully?) or should this code:

	if (start & 511)
 		return -EINVAL;
 	if (len & 511)
 		return -EINVAL;

be something more like this (for better sanity checking of your arguments) which will ensure that you don't have implicit conversion issues from unsigned to signed and ensure that the result of adding them together won't either:

	if ((start & 511) || (start > (uint64_t)LLONG_MAX))
 		return -EINVAL;
 	if ((len & 511) ) || (len > (uint64_t)LLONG_MAX))
 		return -EINVAL;
	if (end > (uint64_t)LLONG_MAX)
		return -EINVAL;

My apologies in advance if I've made a mistake when looking at this and my concerns about unsigned values being implicitly converted to signed are unfounded (I would have hoped for compiler warnings about any implicit conversions though).

Thanks
Shane
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/