Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 26 Oct 2018 13:22:05 -0700
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     LKML <linux-kernel@vger.kernel.org>, kernel-team@android.com,
        Anton Vorontsov <anton@enomsg.org>,
        Colin Cross <ccross@android.com>,
        Tony Luck <tony.luck@intel.com>
Subject: Re: [RFC 5/6] pstore: donot treat empty buffers as valid
Message-ID: <20181026202205.GB129228@joelaf.mtv.corp.google.com>
References: <20181026180042.52199-1-joel@joelfernandes.org>
 <20181026180042.52199-5-joel@joelfernandes.org>
 <CAGXu5jKtZjZVr=7dAHQPU0OGtBpEKDZBO68-RTvFLbK6qB+cEA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAGXu5jKtZjZVr=7dAHQPU0OGtBpEKDZBO68-RTvFLbK6qB+cEA@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Oct 26, 2018 at 08:39:13PM +0100, Kees Cook wrote:
> On Fri, Oct 26, 2018 at 7:00 PM, Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> > pstore currently calls persistent_ram_save_old even if a buffer is
> > empty. While this appears to work, it is simply not the right thing to
> > do and could lead to bugs so lets avoid that. It also prevent misleading
> > prints in the logs which claim the buffer is valid.
> 
> I need to be better convinced that a present zero length record is the
> same as a non-present record. This seems true, but there is
> potentially still metadata available from a backend. What were the
> misleading prints in logs?

I got something like:
found existing buffer, size 0, start 0

When I was expecting:
no valid data in buffer (sig = ...)

The other thing is a call to persistent_ram_zap is also prevented on the
buffer, which prevent zero-initialize prz->ecc_info.par. Since we are
dropping patch 6/6, the zap will not happen. But I'm not super familiar with
the ecc bits of this code to say that if that's an issue.

About the present zero-length record, I would argue that it should not be
"present" at all. When the system first boots, the record is not present but
the signatures are initialized, then on reboots because the signatures were
initialized, the buffer appears as valid even if it was unused. So for dmesg,
all the max_dump_cnt number of buffers would appear as if they are valid
which is a bit strange because there was no crash at all so it should be in
the same state on reboot, as if there was no crash. That could be a matter of
perspective though so I leave it you how you prefer to do it :)

thanks,

- Joel