linux-kernel - Re: [PATCH v2] statx: optimize copy of struct statx to userspace

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: <F7337E43-D0B7-4196-8BB9-8E292C582C3B@dilger.ca>
Date:   Sun, 12 Mar 2017 22:34:38 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     Al Viro <viro@...IV.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        David Howells <dhowells@...hat.com>,
        linux-kernel@...r.kernel.org, Eric Biggers <ebiggers@...gle.com>
Subject: Re: [PATCH v2] statx: optimize copy of struct statx to userspace

On Mar 11, 2017, at 11:01 PM, Eric Biggers <ebiggers3@...il.com> wrote:
> 
> On Sat, Mar 11, 2017 at 08:02:06PM -0800, Eric Biggers wrote:
>> On Sun, Mar 12, 2017 at 02:29:27AM +0000, Al Viro wrote:
>>> 
>>> Oh, I agree that multiple __put_user() are wrong; I also agree that bulk
>>> copy is the right approach (when we get the unsafe stuff right, we can
>>> revisit that, but I suspect that on quite a few architectures a bulk copy
>>> will still give better time, no matter what).
>>> 
>>>> If padding is a concern at all (AFAICS it's not actually an issue now
>>>> with struct statx, but people tend to have different opinions on how
>>>> careful they want to be with padding), then I think we'll just have to
>>>> start by memsetting the whole struct to 0.
>>> 
>>> My point is simply that it's worth a comment in that code.
>> 
>> Okay, thanks.  I'll add a comment about the padding assumption, and I think
>> I'll take the suggestion to use a designated initializer.  Then at least
>> all *fields* get initialized by default.  And if in the future someone
>> wants to conditionally initialize fields, then they can use ?: or they can
>> do it after the initializer.  Either way, at least they won't be able to
>> forget to zero some field.
> 
> Okay, well, I may have changed my mind again...  I tried the designated
> initializer on x86_64 with gcc 4.8 and 6.3, and also on arm64 with gcc 4.8.
> In each case, it was compiled into first zeroing all 256 bytes of the struct,
> just like memset(&tmp, 0, sizeof(tmp)).  Yes, this was with
> CC_OPTIMIZE_FOR_PERFORMANCE=y.  So I think we might as well just write the
> full memset(), making it completely clear that everything is initialized.
> (This is especially useful for people who are auditing code paths like this
> for information leaks.)  Also, a smart compiler could potentially optimize
> away parts of the memset() anyway...

Not that it is a huge deal either way, but I'd think it is harder for the
compiler to optimize across a function call boundary like memset() vs. a
struct initialization in the same function where it can see that all but
a few of the fields are being overwritten immediately before they are used.

I don't think the designated initializer is any less clear to the reader
that the struct is zeroed out compared to using memset().  Possibly the
best compromise is to use a designated initializer that specifies all of
the known fields, and leaves it to the compiler to initialize unset fields
or padding.  That avoids double zeroing without any risk of exposing unset
fields to userspace:

static int cp_statx(const struct kstat *stat, struct statx __user *buffer)
{
	struct statx tmp = {
		.stx_mask = stat->result_mask;
		.stx_blksize = stat->blksize;
		.stx_attributes = stat->attributes;
		.stx_nlink = stat->nlink;
		.stx_uid = from_kuid_munged(current_user_ns(), stat->uid);
		.stx_gid = from_kgid_munged(current_user_ns(), stat->gid);
		.stx_mode = stat->mode;
		.stx_ino = stat->ino;
		.stx_size = stat->size;
		.stx_blocks = stat->blocks;
		.stx_atime.tv_sec = stat->atime.tv_sec;
		.stx_atime.tv_nsec = stat->atime.tv_nsec;
		.stx_btime.tv_sec = stat->btime.tv_sec;
		.stx_btime.tv_nsec = stat->btime.tv_nsec;
		.stx_ctime.tv_sec = stat->ctime.tv_sec;
		.stx_ctime.tv_nsec = stat->ctime.tv_nsec;
		.stx_mtime.tv_sec = stat->mtime.tv_sec;
		.stx_mtime.tv_nsec = stat->mtime.tv_nsec;
		.stx_rdev_major = MAJOR(stat->rdev);
		.stx_rdev_minor = MINOR(stat->rdev);
		.stx_dev_major = MAJOR(stat->dev);
		.stx_dev_minor = MINOR(stat->dev);
	};

	return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
}

Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)