[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Wed, 22 Nov 2017 11:41:19 +0530
From: Kaiwan N Billimoria <kaiwan.billimoria@...il.com>
To: "Tobin C. Harding" <me@...in.cc>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH v1] scripts: leaking_addresses.pl: add support for 32-bit
kernel addresses
Thanks Tobin, for your detailed comments.
On Wed, Nov 22, 2017 at 5:29 AM, Tobin C. Harding <me@...in.cc> wrote:
> You don't typically need [xxx v1] for version 1, the v1 is implicit.
>
> Please use the git brief description prefix that is already in use i.e
>
> leaking_addresses: add support for 32-bit kernel addresses
Ok..
> On Tue, Nov 21, 2017 at 01:28:14PM +0530, kaiwan.billimoria@...il.com wrote:
>> - support for ARM-32
>
> Sure, we can do this later.
Righto
>
>> - programatically query and set the PAGE_OFFSET based on arch (it's currently
>> hard-coded)
>
> Let's do this straight away, it will be much nicer.
Yes, will work on it..
>> 2. Minor edit:
>> the '--raw', '--suppress-dmesg', '--squash-by-path' and
>> '--squash-by-filename' option switches are only meaningful
>> when the '----input-raw=' option is used. So, indent the 'Help' screen lines
>> to reflect the fact.
>
> This is a different change to the architecture stuff so should be in a
> separate patch. You could do both as a series if you like. Off the top
> of my head I have never seen options output like this, but if you have,
> I'm willing to accept your view. You are correct that the options
> mentioned are only use in conjuncture with '--input-raw' so some way of
> showing this would be nice.
I realize this; so, yeah, will make the next one a series and put this
in the 2nd..
>>
>> +my $bit_size = 64; # Check 64-bit kernel addresses by default
>
> This global is unnecessary. You already have is_ix86_32() so you can just
> use that.
>From your later comments, I think you see that using this global is necessary.
> Please use kernel coding style
>
> $bit_size = 32;
Ok..
> Bonus points, you uncovered a bug in the current script `if (is_x86_64)`
> was missing the parenthesis!
Yeah :-)
>
>> + if ($match =~ '\b(0x)?(f|F){8}\b') {
>> + return 1;
>> + }
>
> So, may_leak_address() and is_false_positive() are tightly coupled and
> not really that nice. Once we add 32 bit support it gets worse. Going
> forwards, we can either add your 32 bit work then refactor both
> functions or you can refactor them as you add the 32 bit stuff. I'm open
> to either.
Yes I agree. Having said that, I'll leave it on the back burner for now..
>Some things to note
>
> - The mask stuff (all 1's) should have an all 0's regex also.
Well, once we determine the address is >= PAGE_OFFSET, it's
automatically apparent that it's not 0, yes?
> - The mask stuff should probably be closer to the mask stuff for 64
> bit. It's not immediately apparent a clean way to do this though.
> - It's not immediately apparent if an address less that PAGE_OFFSET is a
> false positive or should be caught in leaks_address().
Hmm only thing I can think of offhand- on many ARM-32's, the kernel
module space is below
PAGE_OFFSET; we'd have to take that into consideration of course.
Anything else < PAGE_OFFSET and a kernel address? Anyone?
> - Do we need 32 bit equivalents for
>
> if ($line =~ '\bKEY=[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b' or
> $line =~ '\b[[:xdigit:]]{14} [[:xdigit:]]{16} [[:xdigit:]]{16}\b') {
>
Ok am unclear on what exactly the above achieves.. could you pl throw
some light on it, thanks..
>
> Your patch did not apply, the problem looks to be in the code section
> above. You can see that there is no removed line. For next spin please
> check your patch applies on top of the 'leaks' branch (which now
> includes the fix for the bug you found).
Yes, sorry about that; will do..
> I have one more comment that should have been at the top but I did not
> want to confuse things. Typically, the git brief description should be
> limited to 50 characters. If you do decide to split this patch into two
> and use the prefix suggested you may like to change the git brief
> description but don't feel you have to. If you do decide to do this, your
> next patch set will be a version 1 again. I may be wrong but I never
> increment a patch version if the subject line changes (excluding
> contents of [] ).
Right. I plan to send the next one as a 2 patch series; will keep the
git prefix you suggest
(and as Sub changes, will not label the version).
>
> thanks,
> Tobin.
Thanks,
Kaiwan.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
