From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754359AbcHAQf1 (ORCPT ); Mon, 1 Aug 2016 12:35:27 -0400 Received: from mail-wm0-f45.google.com ([74.125.82.45]:38049 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754115AbcHAQeX (ORCPT ); Mon, 1 Aug 2016 12:34:23 -0400 Date: Mon, 1 Aug 2016 18:33:28 +0200 From: Ivan Delalande To: Vincent Brillault Cc: Andrew Morton , Petr Mladek , Sergey Senozhatsky , Andrey Ryabinin , Kees Cook , Thierry Reding , Geliang Tang , Tejun Heo , linux-kernel@vger.kernel.org Subject: Re: kernel/printk/printk.c: Invalid access when buffer wraps around? Message-ID: <20160801163328.GI26511@ycc.fr> References: <89c15437-5a97-68b1-d83f-097f3b047559@cern.ch> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <89c15437-5a97-68b1-d83f-097f3b047559@cern.ch> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Mon, Jul 25, 2016 at 11:22:24AM +0200, Vincent Brillault wrote: > While working on a kernel module that basically forks > kernel/printk/printk.c to create its own buffer, I believe I have found > a bug in how printk wraps around its buffer: In a very specific case, a > reader, trying to read what was supposed to be a zeroed header > indicating that the buffer wrapped, might read random data from another > message that has overridden it. The root of the problems seems to be > that the zeroed header placed at the end of the buffer is not protected > by the sequence number checks. > > [...] > > Due to scheduling/reader choice, the reader now stops. > A 20 character-long message arrives. The buffers removes messages from before > and wraps arround: > 88888888888888888883333333444444444555555555556666666677770........... > > And we now have: > log_first_seq = 3, log_next_seq = 9 > log_first_idx = 20, log_next_idx = 20 > > Let's continue filling the buffer, without wrapping: > 88888888888888888889999999999999999999aaaaaaaaaaaaabbbbbbbbbbbbbb..... > > We now have: > log_first_seq = 8, log_next_seq = 0xc > log_first_idx = 0, log_next_idx = 65 > > And now the reader restarts, it still have the same sequence number, '8' > As `seq < log_first_seq` is false, it will consider that this message is valid > and continue. Unfortunately, it's first action will be `log_from_idx(idx)`, > which will first try to read the header of the message at the offset `idx`. > This header, which was supposed to be zeroed due to the wrap-around, has now > been overritten and thus contain random junk, which might ultimately lead > to a kernel panic (wrong memory access from the reader). > > As I understand it, the underlying problem is that the zeroed-header is > not properly protected by the sequence number cheks. Thanks for your very detailled write-up but I believe you are mistaken on the value of log_first_idx between these two last steps you describe. To detail even more: - we want to store bbb… and we have first_seq=7, first_idx=57, - first loop in log_make_free_space(), not enough space, so we move the next one, first_seq=8, first_idx= *59* (log_next check of msg->len is for record 7), - second loop in log_make_free_space(), logbuf_has_space uses first_idx=59 and will still return false, so we go to the next record once again and end up with first_seq=9, first_idx=23. So I think the situation you describe cannot happen, first_seq will always be incremented when we find the 0-filled record and wrap around, and so all the later checks of "*_seq < log_first_seq" will reset all the readers indexes. Did I miss anything? -- Ivan "Colona" Delalande Arista Networks