linux-kernel - Re: [PATCH] romsignature/checksum cleanup

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: <45A0CFC6.3030707@gmail.com>
Date:	Sun, 07 Jan 2007 11:47:34 +0100
From:	Rene Herman <rene.herman@...il.com>
To:	Jeremy Fitzhardinge <jeremy@...p.org>
CC:	Zachary Amsden <zach@...are.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Andrew Morton <akpm@...l.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] romsignature/checksum cleanup

On 01/07/2007 11:20 AM, Jeremy Fitzhardinge wrote:

> Rene Herman wrote:

>> How is it for efficiency? I thought it was for correctness.
>> romsignature is using probe_kernel_adress() while all other accesses
>> to the ROMs there aren't.
>>
>> If nothing else, anyone reading that code is likely to ask himself the
>> very same question -- why the one, and not the others.
> 
> Well, I was wondering about all the uses of __get_user; why not
> probe_kernel_address() everywhere?

It's just a manual version of probe_kernel_adress():

#define probe_kernel_address(addr, retval)              \
         ({                                              \
                 long ret;                               \
                 mm_segment_t old_fs = get_fs();         \
                                                         \
                 set_fs(KERNEL_DS);                      \
                 pagefault_disable();                    \
                 ret = __get_user(retval, [ ... ]);      \
                 pagefault_enable();                     \
                 set_fs(old_fs);                         \
                 ret;                                    \
         })

Doing the set_fs() and pagefault_{disable,enable} calls for every single 
byte during the checksum seems rather silly. The patch as posted has the 
set_fs() and pagefault_ calls only once in probe_roms() (as said when 
posted, I'm not sure the pagefault calls are still useful now that it's 
no longer a generic function/macro, but used directly at probe_roms() time).

> I think its reasonable to assume that if the signature is mapped and 
> correct, then everything else is mapped.  That's certainly the case
> for Xen, which is why I added it.  If you think this is unclear, then
> I think a comment to explain this rather than code changes is the 
> appropriate fix.

I disagree I'm afraid. Given what __get_user compiles to (nothing more 
than a .fixup entry, basically) they're largely "free" and it makes the 
code completely obvious: "If you're touching this, do so via __get_user 
and not directly" and frees it from any assumptions, however reasonable 
or unreasonable.

Would you _mind_ if I submit it? If not, if you could comment on whether 
or not these pagefault calls are still useful, that would be great. The 
comment at probe_kernel_address() says:

  * We ensure that the __get_user() is executed in atomic context so that
  * do_page_fault() doesn't attempt to take mmap_sem. This makes
  * probe_kernel_address() suitable for use within regions where the
  * caller already holds mmap_sem, or other locks which nest inside
  * mmap_sem

This sounds like it might be nonsensical at probe_roms() time, but I'm 
not familiar with these virtualized environments -- I do not know which 
assumptions break.

Patch attached again for reference...

Rene.

View attachment "probe_kernel_address.diff" of type "text/plain" (2543 bytes)