linux-kernel - Re: x86-32: clean up rwsem inline asm statements

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, 13 Jan 2010 13:04:09 -0800
From:	"H. Peter Anvin" <hpa@...or.com>
To:	George Spelvin <linux@...izon.com>
CC:	linux-kernel@...r.kernel.org, schwab@...ux-m68k.org,
	torvalds@...ux-foundation.org
Subject: Re: x86-32: clean up rwsem inline asm statements

On 01/13/2010 11:58 AM, George Spelvin wrote:
>> As far as I can tell, very few of these assembly statements actually
>> need a size at all -- the very first inc statement is purely to memory,
>> and as such it needs a size marker, but almost any operation which is
>> register-register or register-memory will simply take its size from the
>> register operand.  For those, it seems cleaner to simply drop the size
>> suffix, and in fact this is the style we have been pushing people
>> towards (use the suffix where mandatory or where the size is fixed
>> anyway, to help catch bugs; use no suffix where the size can vary and is
>> implied by the operands.)
> 
> The one thing is that for a register-memory operation, using the
> size of the memory operand can catch bugs where it doesn't match
> the size of the register operand.
> 
> GCC's inline asm doesn't make operand size very implicit, and it's
> awkward to cast output operands, so there's a potential for bugs.
> I especially get nervous when the operand itself is an immediate
> constant, as I can't remember the ANSI rules for the type very well.
> (Quick: is 0x80000000 an unsigned 32-bit int or a signed 64-bit one?
> What about 2147483648 or 1<<31?)
> 
> Since this is mostly inline functions, it's not so big a problem, but
> I'd consider something like:
> 
> static inline void __up_write(struct rw_semaphore *sem)
> {
>        unsigned long tmp;
> 	asm volatile("# beginning __up_write\n\t"
> 		     LOCK_PREFIX "  xadd%z0	%1,(%2)\n\t"
> 		     /* tries to transition
> 			0xffff0001 -> 0x00000000 */
> 		     "  jz	1f\n"
> 		     "  call call_rwsem_wake\n"
> 		     "1:\n\t"
> 		     "# ending __up_write\n"
>        	     : "+m" (sem->count), "=d" (tmp)
>        	     : "a" (sem), "1" (-RWSEM_ACTIVE_WRITE_BIAS)
>        	     : "memory", "cc");
> }
> 
> Just in case the size of -RWSEM_ACTIVE_WRITE_BIAS doesn't match that
> of sem->count.  It'll explode when you try to run it, of course, but
> there's something to be said for compile-time errors.

There are a number of things that can be done better... for one thing,
"+m" (sem->count) and "a" (sem) is just bloody wrong.  The right thing
would be "a" (&sem->count) for proper robustness.

There is no real point in being concerned about the type of immediates,
because the immediate type isn't really used... it shows up as a literal
in the assembly language.  However, if you're really concerned, the
right thing to do is to do a cast in C, not playing games with the assembly.

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