mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Eric Dumazet <dada1@cosmosbay.com>
Cc: security@kernel.org, tytso@mit.edu,
	Herbert Xu <herbert@gondor.apana.org.au>,
	John Reiser <jreiser@BitWagon.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-sparse@vger.kernel.org, mpm@selenic.com,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [Security] Signed divides vs shifts (Re: /dev/urandom uses uninit bytes, leaks user data)
Date: Mon, 17 Dec 2007 10:28:38 -0800 (PST)	[thread overview]
Message-ID: <alpine.LFD.0.9999.0712171009570.21557@woody.linux-foundation.org> (raw)
In-Reply-To: <20071217185557.0b501e23.dada1@cosmosbay.com>



On Mon, 17 Dec 2007, Eric Dumazet wrote:
> 
> while 
> 
> long *mid(long *a, long *b)
> {
> 	return ((a - b) / 2u + a);
> }

This is exactly what I'm talking about. That "2u" is TOTALLY POINTLESS. 
It's an "unsigned int", but since (a-b) will be of type ptrdiff_t, and is 
*wider* on a 64-bit architecture (it's the same as "long" on x86-64), then 
the 2u will just be converted to "long", and be signed again! 

So you thought that you did an unsigned divide, but you did no such thing. 

If you change the "2u" to a "2ul", it works again, and you get

	mid:
	        movq    %rdi, %rax
	        subq    %rsi, %rax
	        sarq    %rax
	        andq    $-8, %rax
	        addq    %rdi, %rax
	        ret

which is the code you wanted. But quite frankly, you could just have 
written it with a shift to start with, and avoided the subtle type issue, 
although gcc then generates

        movq    %rdi, %rax
        subq    %rsi, %rax
        sarq    $4, %rax
        leaq    (%rdi,%rax,8), %rax
        ret

instead. Of course, this all *does* still have subtle sign issues, because 
the "a-b" part implies a signed divide in itself, which is why you see 
that "sarq" in he first place (rather than a "shrq"). 

Signed divides are hard. The "a-b" pointer subtraction is actually cheaper 
than a general signed divide by sizeof, since the compiler can then assume 
that the two pointers are mutually aligned, which is why gcc can generate 
just a single "sarq" instead of having to do an extra "add negative bit" 
thing to get the rounding right.

[ So Al, when you said that

	(a-b)

  is equivalent to

	((char *)a-(char *)b)/4

  for a "int *" a and b, you're right in the sense that the *result* is 
  the same, but the code generation likely isn't. The "a-b" thing can (and 
  does) allow the compiler to avoid the whole "align up for signed 
  numbers" thing, and the difference in code generation is clear:

	subq    %rsi, %rdi
	sarq    $2, %rdi

  vs

	subq    %rsi, %rdi
	leaq    3(%rdi), %rax
	testq   %rdi, %rdi
	cmovs   %rax, %rdi
	sarq    $2, %rdi

  exactly because the first case *knows* that the low two bits have to be 
  zero, and thus there is no rounding issue. ]

			Linus

  parent reply	other threads:[~2007-12-17 18:32 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-12-14 19:34 /dev/urandom uses uninit bytes, leaks user data John Reiser
2007-12-14 20:13 ` Matt Mackall
2007-12-14 20:45   ` John Reiser
2007-12-14 23:23     ` Theodore Tso
2007-12-15  0:30       ` John Reiser
2007-12-15  4:32         ` Theodore Tso
2007-12-17 16:30           ` John Reiser
2007-12-17 17:36             ` Theodore Tso
2007-12-18  0:52               ` Andy Lutomirski
2007-12-18  3:05                 ` Theodore Tso
2007-12-18  3:13                   ` David Newall
2007-12-18  3:46                     ` Theodore Tso
2007-12-18  4:09                       ` David Newall
2007-12-18  4:23                         ` Theodore Tso
2007-12-19 22:43                           ` Bill Davidsen
2007-12-19 22:40                         ` Bill Davidsen
2007-12-20  4:18                       ` Andrew Lutomirski
2007-12-20 20:17                         ` Phillip Susi
2007-12-21 16:10                           ` Andrew Lutomirski
2007-12-22  1:14                             ` Theodore Tso
2007-12-26 18:30                             ` Phillip Susi
2007-12-20 20:36                         ` Theodore Tso
2007-12-27 10:44                           ` Pavel Machek
2007-12-18  5:12                 ` David Schwartz
2007-12-17 20:59             ` David Schwartz
2007-12-15  7:13         ` Herbert Xu
2007-12-15 16:30           ` Matt Mackall
2007-12-17 17:28           ` Signed divides vs shifts (Re: [Security] /dev/urandom uses uninit bytes, leaks user data) Linus Torvalds
2007-12-17 17:48             ` Al Viro
2007-12-17 17:55             ` Eric Dumazet
2007-12-17 18:05               ` Ray Lee
2007-12-17 18:10                 ` Eric Dumazet
2007-12-17 18:12                   ` Ray Lee
2007-12-17 18:23               ` Al Viro
2007-12-17 18:28               ` Linus Torvalds [this message]
2007-12-17 19:08                 ` [Security] Signed divides vs shifts (Re: " Al Viro

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LFD.0.9999.0712171009570.21557@woody.linux-foundation.org \
    --to=torvalds@linux-foundation.org \
    --cc=akpm@linux-foundation.org \
    --cc=dada1@cosmosbay.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=jreiser@BitWagon.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sparse@vger.kernel.org \
    --cc=mpm@selenic.com \
    --cc=security@kernel.org \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®