From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753639AbcJDRzr (ORCPT ); Tue, 4 Oct 2016 13:55:47 -0400 Received: from mail-pa0-f67.google.com ([209.85.220.67]:33691 "EHLO mail-pa0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751223AbcJDRzq (ORCPT ); Tue, 4 Oct 2016 13:55:46 -0400 Subject: Re: [RFC PATCH v1 2/2] printk: external log buffer (CONFIG_LOGBUFFER) To: Petr Mladek References: <1475196956-32274-1-git-send-email-sean_hudson@mentor.com> <1475196956-32274-3-git-send-email-sean_hudson@mentor.com> <20160930123900.GL26796@pathway.suse.cz> <5ac629d8-802e-a982-e4e9-f3cba83aef8c@mentor.com> <20161004112707.GC13369@pathway.suse.cz> Cc: linux-kernel@vger.kernel.org, Andrew Morton , Thomas Gleixner , Yang Shi , Ingo Molnar , Prarit Bhargava , Rasmus Villemoes , "Paul E. McKenney" , Yaowei Bai , Kees Cook , Andrey Ryabinin , Johannes Weiner , Tejun Heo , Arnd Bergmann , Ard Biesheuvel , Thomas Garnier , Nicolas Pitre , Sergey Senozhatsky , Borislav Petkov , Tim Bird , Steven Rostedt , Grant Likely From: Sean Hudson Message-ID: Date: Tue, 4 Oct 2016 12:55:35 -0500 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20161004112707.GC13369@pathway.suse.cz> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="352nWiLg1nKRswmCMU9mqX7bQTr3fPI4d" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --352nWiLg1nKRswmCMU9mqX7bQTr3fPI4d Content-Type: multipart/mixed; boundary="oNxQjIgeo8hJbSDwH1Wmq3sheDdEE1eOn"; protected-headers="v1" From: Sean Hudson To: Petr Mladek Cc: linux-kernel@vger.kernel.org, Andrew Morton , Thomas Gleixner , Yang Shi , Ingo Molnar , Prarit Bhargava , Rasmus Villemoes , "Paul E. McKenney" , Yaowei Bai , Kees Cook , Andrey Ryabinin , Johannes Weiner , Tejun Heo , Arnd Bergmann , Ard Biesheuvel , Thomas Garnier , Nicolas Pitre , Sergey Senozhatsky , Borislav Petkov , Tim Bird , Steven Rostedt , Grant Likely Message-ID: Subject: Re: [RFC PATCH v1 2/2] printk: external log buffer (CONFIG_LOGBUFFER) References: <1475196956-32274-1-git-send-email-sean_hudson@mentor.com> <1475196956-32274-3-git-send-email-sean_hudson@mentor.com> <20160930123900.GL26796@pathway.suse.cz> <5ac629d8-802e-a982-e4e9-f3cba83aef8c@mentor.com> <20161004112707.GC13369@pathway.suse.cz> In-Reply-To: <20161004112707.GC13369@pathway.suse.cz> --oNxQjIgeo8hJbSDwH1Wmq3sheDdEE1eOn Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 10/4/2016 6:27 AM, Petr Mladek wrote: > On Fri 2016-09-30 23:06:49, Sean Hudson wrote: >> On 9/30/2016 7:39 AM, Petr Mladek wrote: >>> >>> Is the bootloader allowed to write to the shared buffer from this >>> point on? How will be synchronized the writes from the kernel and >>> the bootloader? >> >> I am a bit confused by this question. I assume that the kernel and >> bootloader will not be accessing the same log space simultaneously. >> While each one is running, they will utilize the same data structures = to >> keep track of current entries. >=20 > They both will write new messages at the end of the buffer. Therefore > they both might want to write to exactly the same location. Also if > the buffer is full, the oldest messages are overwritten. Therefore > even reading must be synchronized against writes. >=20 > These operations are synchronized using logbuf_lock on the kernel side.= > I do not know much about bootloaders but I guess that the bootloader > will not be able to use the logbuf_lock. So the question is how > the bootloader and kernel will synchronize the access to the shared > buffer. >=20 > Or did I miss anything? The state of the log is preserved in the log control block, or lcb, which is passed when the boot loader passes control to the kernel. In order to make the transition as seamless as possible, the boot loader appends entries in the same way that the kernel does and overwrites the oldest entries once the ring buffer is full. It also updates tracking indices in the lcb. When the boot loader passes control to the kernel, the kernel adopts the lcb and appends entries using that structure. In this way, the lcb provides continuity between the boot loader and the kernel for the state of the buffer. Note, there is a small window of time during early boot in which the kernel puts entries into the default location, a staticlly declared char[]. These entries are appended to the external location once setup_ext_logbuff() is called from init/main.c. (I'd love to close that gap entirely, but all solutions I've tried involve arch specific code, which I'm trying to avoid.) >>> How protected will the address of the shared buffer? Would not >>> it open a security hole? We need to make sure that non-privileged >>> user or non-trusted application must not read sensitive data >>> from the log or break the structure of the log by writing >>> invalid data. >> >> There is no notion of protection of the address. However, additional >> checking could be added. Currently, there is potential for corrupt log= >> entries from the bootloader causing the kernel to crash. >> I can't think of a way to prevent that from happening. >=20 > IMHO, the only way is to revisit all locations when the log buffer > is accessed and add all the needed consistency checks. We must make > sure that we do not access outside of the buffer. Also the log_*_seq > numbers are supposed to grow linearly... Adding an explicit walk of the external log entries should be trivial. That will only validate that the passed in log entries are consistent internally. I will be submitting a v2 patch shortly that fixes an error that kernel-ci caught. (if CONFIG_PRINTK is disabled, compilation fails) I'll see about adding that check at the same time. >> Also, as you point out, a bootloader could read log contents from >> the shared region. Of course, in order for either to happen, >> the bootloader would already be compromised and I'm not sure that >> reading log entries or crashing the kernel is such a big consideratio= n. >=20 > The log might contain addresses of some kernel structures, for example > in some error messages. For this reason, only a privileged users are > able to read it. We must make sure that they will not get accessible > for a non-privileged user, for example via an address to the shared > buffer in the bootloader config. Ahhh, if I am understanding correctly, your concern is that an unprivileged program in user space would be able to gain access directly to the log entries via the external buffer location. I haven't tested that scenario, so I'm not sure if the memory reservation via the DT protects that memory from user space. I'll have to check on that. In the meantime, if you know of standard ways to protect memory from user space access, please let me know. > BTW: You did not answered the question about how the bootloader would > know the right version of the log format. I am afraid that we do not > want to maintain backward compatibility on the kernel side. The printk > code already is too complex. This is a matter of magic numbers. Uboot has two preexisting log structure versions. Since the kernel did not have a version, I used LOGBUFF_LOG_VERSION =3D 3, if CONFIG_LOGBUFFER is enabled. So, if the boot loader passes the command line parameter 'lcb_phys_addr' and the contents of that address pass the validation checks found in is_valid_external_cb(), then the external buffer is used. Those checks include matching the version number, structure sizes match, etc. Otherwise, the external buffer is ignored. This should allow for the kernel to change in the future, e.g. v4, and continue to run correctly even if an older boot loader passes a v3 version of the log entries. > Also you did not answered the question about your plans. I wonder > which bootloader would be able to use such a feature. >=20 Sorry about that, I missed the question in your previous reply. I am preparing to submit a patch set to mainline uBoot that adds this capabili= ty. --oNxQjIgeo8hJbSDwH1Wmq3sheDdEE1eOn-- --352nWiLg1nKRswmCMU9mqX7bQTr3fPI4d Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX8+0dAAoJEC2KalNZVFwRh7oP/jaXhNeLMehoO1D3HtTIbbRR Nt9S76DOL7UBrzKr0UOfzO6BM5yGm3qwyZV8gL4FZCkmNm0+UXw6+a3/yVemFmgi KgpSvygoPgtBDJYhRQK+8SCKR2hF3aJnNZ+vAUJadO8N84J0IxOBE8Lc0cTSI1ZV xN5rfJZdidu6zxCDkYFNqKQOa+LahkQ543k8reesQQX1AeSFEjfRNlviMAYKcswO 0Ruq28sRs1dZQZyqbWZWbw2T1bfHh7Abr1Q8d6Qcg6kVzOkydJKkN3CNcU1YYxgr wEtIBVFt0kOcRKmjlOTxTUz2I86ShXqDks3MXomzhuyAp+QOXFT67KTvxGfsTdJz vIPeN4RJlVSc+JjNR/9IPYERjr7NVi1KomGKeTl2RgkKuYTswc1pDCA/ccSYPvJy 39eWba0soskMNtio7dPwKr9XrnXyEtVIS5+4pqiqIBQwitoUEMV/awcdn6TETRwF Rf04Zci/C/Ts23aqhBM+cQdn4Ln0AWYwUotwlTZiuBc04Ixdb+MxrSxaVP11Qe9k ixOwxvvH+xSztruPEBVUCbGPn58LqjaGEY23/INaJKT/YhZRaPAkgje9VfX7hNQr LWZkyn/7UwQVlUp9tWPgXs1IgXSRIqNJyBxfuC+Kl2EGNTXGaYJBDC5J8tkTpD1K +C2acvwQYC+AAwxz02Gf =yH/r -----END PGP SIGNATURE----- --352nWiLg1nKRswmCMU9mqX7bQTr3fPI4d--