linux-kernel - Re: [PATCH 1/6] lib/scatterlist: Tidy types and fix overflow checking in sgl_alloc

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]

Message-ID: <1520439036.2890.13.camel@wdc.com>
Date:   Wed, 7 Mar 2018 16:10:37 +0000
From:   Bart Van Assche <Bart.VanAssche@....com>
To:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "tursulin@...ulin.net" <tursulin@...ulin.net>
CC:     "tvrtko.ursulin@...el.com" <tvrtko.ursulin@...el.com>,
        "hare@...e.com" <hare@...e.com>,
        "jthumshirn@...e.de" <jthumshirn@...e.de>,
        "axboe@...nel.dk" <axboe@...nel.dk>
Subject: Re: [PATCH 1/6] lib/scatterlist: Tidy types and fix overflow checking
 in sgl_alloc_order

On Wed, 2018-03-07 at 12:47 +0000, Tvrtko Ursulin wrote:
> sgl_alloc_order explicitly takes a 64-bit length (unsigned long long) but
> then rejects it in overflow checking if greater than 4GiB allocation was
> requested. This is a consequence of using unsigned int for the right hand
> side condition which then natuarally overflows when shifted left, earlier
> than nent otherwise would.
> 
> Fix is to promote the right hand side of the conditional to unsigned long.

Agreed.

> It is also not useful to allow for 64-bit lenght on 32-bit platforms so
> I have changed this type to a natural unsigned long. Like this it changes
> size naturally depending on the architecture.

I do not agree. Although uncommon, it is possible that e.g. a SCSI initiator
sends a transfer of more than 4 GB to a target system and that that transfer
must not be split. Since this code is used by the SCSI target, I think that's
an example of an application where it is useful to allow allocations of more
than 4 GB at once on a 32-bit system.

> 2.
> 
> elem_len should not be explicitly sized u32 but unsigned int, to match
> the underlying struct scatterlist nents type. Same for the nent_p output
> parameter type.

Are you sure it is useful to support allocations with an order that exceeds
(31 - PAGE_SHIFT)? Since memory gets fragmented easily in the Linux kernel I
think that it's unlikely that such allocations will succeed.

> I renamed this to chunk_len and consolidated its use throughout the
> function.

Please undo this change such that the diff remains as short as possible.

> -void sgl_free_n_order(struct scatterlist *sgl, int nents, int order)
> +void sgl_free_n_order(struct scatterlist *sgl, unsigned int nents,
> +		      unsigned int order)
>  {
>  	struct scatterlist *sg;
>  	struct page *page;
> -	int i;
> +	unsigned int i;
>  
>  	for_each_sg(sgl, sg, nents, i) {
>  		if (!sg)
> @@ -583,9 +587,9 @@ EXPORT_SYMBOL(sgl_free_n_order);
>   * @sgl: Scatterlist with one or more elements
>   * @order: Second argument for __free_pages()
>   */
> -void sgl_free_order(struct scatterlist *sgl, int order)
> +void sgl_free_order(struct scatterlist *sgl, unsigned int order)
>  {
> -	sgl_free_n_order(sgl, INT_MAX, order);
> +	sgl_free_n_order(sgl, UINT_MAX, order);
>  }
>  EXPORT_SYMBOL(sgl_free_order);

Do you have an application that calls these functions to allocate more than
INT_MAX * PAGE_SIZE bytes at once? If not, please leave these changes out.

Thanks,

Bart.