From: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> To: "Tobin C. Harding" <me@tobin.cc> Cc: Alexander Kapshuk <alexander.kapshuk@gmail.com>, linux-kernel <linux-kernel@vger.kernel.org>, kernel-hardening@lists.openwall.com Subject: Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses Date: Wed, 29 Nov 2017 16:32:54 +0530 [thread overview] Message-ID: <CAPDLWs8ooetPXhuHXyCLZ2LnPRKBf0CSMoESbXMD4sGnLLNFuA@mail.gmail.com> (raw) In-Reply-To: <20171129101640.GC6217@eros> Hi, On Wed, Nov 29, 2017 at 3:46 PM, Tobin C. Harding <me@tobin.cc> wrote: > On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote: >> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote: >> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote: >> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote: >> >> > Options: >> >> > >> >> > - -o, --output-raw=<file> Save results for future processing. >> >> > - -i, --input-raw=<file> Read results from file instead of scanning. >> >> > - --raw Show raw results (default). >> >> > - --suppress-dmesg Do not show dmesg results. >> >> > - --squash-by-path Show one result per unique path. >> >> > - --squash-by-filename Show one result per unique filename. >> >> > - -d, --debug Display debugging output. >> >> > - -h, --help, --version Display this help and exit. >> >> > + -o, --output-raw=<file> Save results for future processing. >> >> > + -i, --input-raw=<file> Read results from file instead of scanning. >> >> > + --raw Show raw results (default). >> >> > + --suppress-dmesg Do not show dmesg results. >> >> > + --squash-by-path Show one result per unique path. >> >> > + --squash-by-filename Show one result per unique filename. >> >> > + --page-offset-32bit=<hex> PAGE_OFFSET value (for 32-bit kernels). >> >> > + --kernel-config-file=<file> Kernel configuration file (e.g /boot/config) >> >> > + -d, --debug Display debugging output. >> >> > + -h, --help, --version Display this help and exit. >> >> > Thanks for the whitespace fixes.. >> >> >> >> Get_page_offset attempts to build a list of config files, which are >> >> then passed into the parsing function for further processing. >> >> This splits up the code to do with the config files between >> >> get_page_offset() and parse_kernel_config_file(). >> >> May I suggest putting the kernel config file processing code into the >> >> parse_kernel_config_file() instead, and let the parsing function >> >> handle the config files and either return the page_offset or an empty >> >> string. >> >> >> >> See below for the proposed implementation. Thanks Alexander.. >> > >> > Nice, this is much better! Thanks. >> > >> >> Apologies for the absence of indentation. >> > >> > Re-posting with indentation, comments in line. >> > >> >> Disclaimer: I did not test-run the code being proposed. >> > >> > I also did not test my comments ;) >> > >> >> sub get_page_offset >> >> { >> >> my $default_offset = "0xc0000000"; >> >> my $page_offset; >> >> >> >> # Allow --page-offset-32bit to over ride. >> >> if ($page_offset_32bit != 0) { >> >> return $page_offset_32bit; >> >> } >> >> >> >> $page_offset = parse_kernel_config_file(); >> >> if ($page_offset ne "") { >> >> return $page_offset >> >> } >> >> >> >> printf STDERR "Failed to parse kernel config files\n"; >> >> printf STDERR "Falling back to %s\n", $default_offset; >> >> >> >> return $default_offset; This "fallback to 0xc0000000" I don't really agree with. Obviously, there are platforms out there that do not use a PAGE_OFFSET value of 0xc0000000. So I think that defaulting to this is kinda presumptive; much better, IMHO, if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via the '--page-offset-32bit=' option switch. What do you say? >> >> } >> >> >> > perhaps >> > my $tmpkconf = '/tmp/tmpkconf'; >> >> my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp >> file is self explanatory. >> Using a variable instead of the filename in this particular context is >> a matter of personal preference. If you prefer to use the variable >> here, it's your call. > > I'm a stickler for no const strings or magic numbers but it's Kaiwan's > patch, if he puts it in with const strings I'll apply it as is :) I'd say in this case it's self-evident; IMO, we could leave it as a const string.. >> > >> >> if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) { >> >> push @config_files, "/tmp/tmpkconf"; >> >> } >> >> } >> > >> > Won't there only ever be a single config file? So if /proc/config.gz is >> > readable we could do >> >> The code above builds a list of config files. >> Assigning to @config_files as shown below would wipe out the config >> files appended to the list so far, would it not? >> So $tmpkconf needs appending to the list. > > You are correct, since the beginning of this function that has been the > algorithm. My observation is that if /proc/config.gz is present then we > don't need to parse the other files so it is better to blow them away. > > I don't know enough about the whole Linux-sphere to know if this is > correct. But it seems reasonable that even if there is more than one way > to look at the running kernels config file they will all be the same, > the system would be pretty broken if they were different. > > So once we have found a readable config file just parse it and be done > with it, no need to loop over any others. Agreed. > thanks, > Tobin. Tobin, am unsure why but I can't seem to apply your patch (on the commit you specified: 4.15-rc1). So have been unable to test so far.. Am copying the patch off the email, saving and trying: linux $ git apply --check ../tobin_patch_28nov17.patch error: patch failed: scripts/leaking_addresses.pl:35 error: scripts/leaking_addresses.pl: patch does not apply linux $ ?? Thanks, Kaiwan.
WARNING: multiple messages have this Message-ID (diff)
From: Kaiwan N Billimoria <kaiwan.billimoria@gmail.com> To: "Tobin C. Harding" <me@tobin.cc> Cc: Alexander Kapshuk <alexander.kapshuk@gmail.com>, linux-kernel <linux-kernel@vger.kernel.org>, kernel-hardening@lists.openwall.com Subject: [kernel-hardening] Re: [PATCH] leaking_addresses: add support for 32-bit kernel addresses Date: Wed, 29 Nov 2017 16:32:54 +0530 [thread overview] Message-ID: <CAPDLWs8ooetPXhuHXyCLZ2LnPRKBf0CSMoESbXMD4sGnLLNFuA@mail.gmail.com> (raw) In-Reply-To: <20171129101640.GC6217@eros> Hi, On Wed, Nov 29, 2017 at 3:46 PM, Tobin C. Harding <me@tobin.cc> wrote: > On Wed, Nov 29, 2017 at 09:59:59AM +0200, Alexander Kapshuk wrote: >> On Tue, Nov 28, 2017 at 11:10 PM, Tobin C. Harding <me@tobin.cc> wrote: >> > On Tue, Nov 28, 2017 at 03:16:24PM +0200, Alexander Kapshuk wrote: >> >> On Tue, Nov 28, 2017 at 8:32 AM, Tobin C. Harding <me@tobin.cc> wrote: >> >> > Options: >> >> > >> >> > - -o, --output-raw=<file> Save results for future processing. >> >> > - -i, --input-raw=<file> Read results from file instead of scanning. >> >> > - --raw Show raw results (default). >> >> > - --suppress-dmesg Do not show dmesg results. >> >> > - --squash-by-path Show one result per unique path. >> >> > - --squash-by-filename Show one result per unique filename. >> >> > - -d, --debug Display debugging output. >> >> > - -h, --help, --version Display this help and exit. >> >> > + -o, --output-raw=<file> Save results for future processing. >> >> > + -i, --input-raw=<file> Read results from file instead of scanning. >> >> > + --raw Show raw results (default). >> >> > + --suppress-dmesg Do not show dmesg results. >> >> > + --squash-by-path Show one result per unique path. >> >> > + --squash-by-filename Show one result per unique filename. >> >> > + --page-offset-32bit=<hex> PAGE_OFFSET value (for 32-bit kernels). >> >> > + --kernel-config-file=<file> Kernel configuration file (e.g /boot/config) >> >> > + -d, --debug Display debugging output. >> >> > + -h, --help, --version Display this help and exit. >> >> > Thanks for the whitespace fixes.. >> >> >> >> Get_page_offset attempts to build a list of config files, which are >> >> then passed into the parsing function for further processing. >> >> This splits up the code to do with the config files between >> >> get_page_offset() and parse_kernel_config_file(). >> >> May I suggest putting the kernel config file processing code into the >> >> parse_kernel_config_file() instead, and let the parsing function >> >> handle the config files and either return the page_offset or an empty >> >> string. >> >> >> >> See below for the proposed implementation. Thanks Alexander.. >> > >> > Nice, this is much better! Thanks. >> > >> >> Apologies for the absence of indentation. >> > >> > Re-posting with indentation, comments in line. >> > >> >> Disclaimer: I did not test-run the code being proposed. >> > >> > I also did not test my comments ;) >> > >> >> sub get_page_offset >> >> { >> >> my $default_offset = "0xc0000000"; >> >> my $page_offset; >> >> >> >> # Allow --page-offset-32bit to over ride. >> >> if ($page_offset_32bit != 0) { >> >> return $page_offset_32bit; >> >> } >> >> >> >> $page_offset = parse_kernel_config_file(); >> >> if ($page_offset ne "") { >> >> return $page_offset >> >> } >> >> >> >> printf STDERR "Failed to parse kernel config files\n"; >> >> printf STDERR "Falling back to %s\n", $default_offset; >> >> >> >> return $default_offset; This "fallback to 0xc0000000" I don't really agree with. Obviously, there are platforms out there that do not use a PAGE_OFFSET value of 0xc0000000. So I think that defaulting to this is kinda presumptive; much better, IMHO, if we just fail and ask the user to pass the "correct" PAGE_OFFSET value via the '--page-offset-32bit=' option switch. What do you say? >> >> } >> >> >> > perhaps >> > my $tmpkconf = '/tmp/tmpkconf'; >> >> my $tmpkconf is almost as long as /tmp/tmpkconf. The name of the tmp >> file is self explanatory. >> Using a variable instead of the filename in this particular context is >> a matter of personal preference. If you prefer to use the variable >> here, it's your call. > > I'm a stickler for no const strings or magic numbers but it's Kaiwan's > patch, if he puts it in with const strings I'll apply it as is :) I'd say in this case it's self-evident; IMO, we could leave it as a const string.. >> > >> >> if (system("gunzip < /proc/config.gz > /tmp/tmpkconf") == 0) { >> >> push @config_files, "/tmp/tmpkconf"; >> >> } >> >> } >> > >> > Won't there only ever be a single config file? So if /proc/config.gz is >> > readable we could do >> >> The code above builds a list of config files. >> Assigning to @config_files as shown below would wipe out the config >> files appended to the list so far, would it not? >> So $tmpkconf needs appending to the list. > > You are correct, since the beginning of this function that has been the > algorithm. My observation is that if /proc/config.gz is present then we > don't need to parse the other files so it is better to blow them away. > > I don't know enough about the whole Linux-sphere to know if this is > correct. But it seems reasonable that even if there is more than one way > to look at the running kernels config file they will all be the same, > the system would be pretty broken if they were different. > > So once we have found a readable config file just parse it and be done > with it, no need to loop over any others. Agreed. > thanks, > Tobin. Tobin, am unsure why but I can't seem to apply your patch (on the commit you specified: 4.15-rc1). So have been unable to test so far.. Am copying the patch off the email, saving and trying: linux $ git apply --check ../tobin_patch_28nov17.patch error: patch failed: scripts/leaking_addresses.pl:35 error: scripts/leaking_addresses.pl: patch does not apply linux $ ?? Thanks, Kaiwan.
next prev parent reply other threads:[~2017-11-29 11:04 UTC|newest] Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-11-28 6:32 [PATCH] leaking_addresses: add support for 32-bit kernel addresses Tobin C. Harding 2017-11-28 6:32 ` [kernel-hardening] " Tobin C. Harding 2017-11-28 13:16 ` Alexander Kapshuk 2017-11-28 13:16 ` [kernel-hardening] " Alexander Kapshuk 2017-11-28 21:10 ` Tobin C. Harding 2017-11-28 21:10 ` [kernel-hardening] " Tobin C. Harding 2017-11-29 7:59 ` Alexander Kapshuk 2017-11-29 7:59 ` [kernel-hardening] " Alexander Kapshuk 2017-11-29 10:16 ` Tobin C. Harding 2017-11-29 10:16 ` [kernel-hardening] " Tobin C. Harding 2017-11-29 11:02 ` Kaiwan N Billimoria [this message] 2017-11-29 11:02 ` Kaiwan N Billimoria 2017-11-29 20:48 ` Tobin C. Harding 2017-11-29 20:48 ` [kernel-hardening] " Tobin C. Harding 2017-12-01 13:03 ` Kaiwan N Billimoria 2017-12-01 13:03 ` [kernel-hardening] " Kaiwan N Billimoria 2017-12-01 13:09 ` kaiwan.billimoria 2017-12-01 13:09 ` [kernel-hardening] " kaiwan.billimoria 2017-12-04 0:11 ` Tobin C. Harding 2017-12-04 0:11 ` [kernel-hardening] " Tobin C. Harding 2017-12-04 4:41 ` kaiwan.billimoria 2017-12-04 4:41 ` [kernel-hardening] " kaiwan.billimoria 2017-12-04 4:55 ` Tobin C. Harding 2017-12-04 4:55 ` [kernel-hardening] " Tobin C. Harding 2017-12-04 5:09 ` Kaiwan N Billimoria 2017-12-04 5:09 ` [kernel-hardening] " Kaiwan N Billimoria 2017-12-04 5:21 ` Kaiwan N Billimoria 2017-12-04 5:21 ` [kernel-hardening] " Kaiwan N Billimoria 2017-12-04 8:21 ` Tobin C. Harding 2017-12-04 8:21 ` [kernel-hardening] " Tobin C. Harding 2017-12-04 10:20 ` kaiwan.billimoria 2017-12-04 10:20 ` [kernel-hardening] " kaiwan.billimoria 2017-12-04 12:37 ` Alexander Kapshuk 2017-12-04 12:37 ` [kernel-hardening] " Alexander Kapshuk 2017-12-04 13:28 ` Kaiwan N Billimoria 2017-12-04 14:08 ` Kaiwan N Billimoria 2017-12-04 14:08 ` [kernel-hardening] " Kaiwan N Billimoria 2017-12-04 20:59 ` Tobin C. Harding 2017-12-04 20:59 ` [kernel-hardening] " Tobin C. Harding 2017-11-29 11:30 ` Alexander Kapshuk 2017-11-29 11:30 ` [kernel-hardening] " Alexander Kapshuk
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=CAPDLWs8ooetPXhuHXyCLZ2LnPRKBf0CSMoESbXMD4sGnLLNFuA@mail.gmail.com \ --to=kaiwan.billimoria@gmail.com \ --cc=alexander.kapshuk@gmail.com \ --cc=kernel-hardening@lists.openwall.com \ --cc=linux-kernel@vger.kernel.org \ --cc=me@tobin.cc \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.