linux-kernel - Re: [PATCH 0/3] Extended file stat functions [ver #2]

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: <84225B35-7365-4DE2-8920-5741011B347C@dilger.ca>
Date:	Wed, 30 Jun 2010 22:57:07 -0600
From:	Andreas Dilger <adilger@...ger.ca>
To:	David Howells <dhowells@...hat.com>
Cc:	viro@...IV.linux.org.uk, smfrench@...il.com, jlayton@...hat.com,
	mcao@...ibm.com, aneesh.kumar@...ux.vnet.ibm.com,
	linux-cifs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, samba-technical@...ts.samba.org,
	sjayaraman@...e.de, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 0/3] Extended file stat functions [ver #2]

On 2010-06-30, at 17:15, David Howells wrote:
> Andreas Dilger <adilger@...ger.ca> wrote:
>>> Passing -1 (or ULONGLONG_MAX) to get everything would be reasonable.
>> 
>> NOOOO.  That is exactly what we _don't_ want, since it makes it impossible
>> for the kernel to actually understand which fields the application is ready
>> to handle.  If the application always uses XSTAT_QUERY_ALL, instead of "-1",
>> then the kernel can easily tell which fields are present in the userspace
>> structure, and what it should avoid touching.
>> 
>> If applications start using "-1" to mean "all fields", then it will work so
>> long as the kernel and userspace agree on the size of struct xstat, but as
>> soon as the kernel understands some new field, but userspace does not, the
>> application will segfault or clobber random memory because the kernel thinks
>> it is asking for XSTAT_QUERY_NEXT_NEW_FIELD|... when it really isn't asking
>> for that at all.
> 
> As long as the field bits allocated in order and the extra results are tacked
> on in bit number order, will it actually be a problem?  Userspace must know how to deal with all the bits up to the last one it knows about; anything beyond that is irrelevant.

The patch you sent seems to get this right, but just for completeness, I'll answer in this thread.  Using the new struct as an example:

	#define XSTAT_REQUEST_GEN		0x00001000ULL
	#define XSTAT_REQUEST_DATA_VERSION	0x00002000ULL

        struct xstat {
                :
                :
		unsigned long long	st_data_version;
		unsigned long long	st_result_mask;
		unsigned long long	st_extra_results[0];
        }

An app "today" would allocate a struct xstat that ends at st_result_mask, and "today's" kernel will not know anything about flags beyond *_DATA_VERSION.  Even if today's app incorrectly uses request_mask = ~0ULL nothing will break until the kernel code changes.

If a future kernel gets a new static field at st_extra_results (say unsigned long long st_ino_high) with a new flag XSTAT_REQUEST_INO_HIGH 0x000040000ULL the kernel will think that the old app is requesting this field, and will fill in the 64-bit field at st_extra_results[1] (which the old app didn't allocate space for, nor does it understand) and may get a segfault, or stack smashing, or random heap corruption.

> What would you have me do?  Return an error if a request is made that the
> kernel doesn't support?  That's bad too.  This can be handled simply by
> clearing the result bit for any unsupported field.

I agree the desirable behaviour is if an app correctly sets request_mask at most to XSTAT_REQUEST__ALL_STATS 0x00003fffULL (or whatever it is at the time the app is compiled that matches the current struct xstat), and if the kernel understands e.g. XSTAT_REQUEST_INO_HIGH or not is irrelevant since the kernel will not touch fields that are not requested.  Likewise, if the application is compiled with a newer/larger XSTAT_REQUEST__ALL_STATS mask than what the kernel understands, the kernel will ignore the flags it doesn't understand.

Cheers, Andreas

--
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/