linux-kernel - Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140929162334.GB30562@pathway.suse.cz>
Date:	Mon, 29 Sep 2014 18:23:34 +0200
From:	Petr Mladek <pmladek@...e.cz>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] seq_file: Fix seq_putc() to be consistent with seq_puts()

On Mon 2014-09-29 11:25:38, Steven Rostedt wrote:
> On Mon, 29 Sep 2014 16:41:22 +0200
> Petr Mladek <pmladek@...e.cz> wrote:
> 
> 
> > This patch moves set_putc() from b) to c) because it leaves mess in the
> > last byte.
> 
> Which removes the "b" section, and makes seq_putc() and seq_puts()
> function the same.
> 
> > 
> > 
> > The semantic looks weird in all categories:
> > 
> > add a) They report error even when there was not a real overflow.
> >     Well, they do not know if there was overflow because
> >     bitmap_scnprintf() does not report if it shrunk
> >     the data or not.
> > 
> > add b) It does not report error even when seq_overflow() might later
> >     report overflow.
> > 
> > add c) Any overflow invalidates the whole buffer because there might
> >     be mess at the end. The overflow is not checked by read functions,
> >     so they allow to read the mess.
> > 
> > 
> > I think that reasonable semantic would be:
> > 
> >   + either signalize overflow another way (extra struct member and
> >     leave count to point to the valid data) or always fill valid
> >     data until the end of the buffer.
> > 
> >     The second variant might be problem if we add only half of
> >     an escape sequence.

Do you prefer:

      + the extra struct member?
      + or always filling some valid data?
      + or invalidating the buffer when the overflow is set?

> > 
> >   + always return error when seq_overflow() would return overflow;
> >     in fact, the full buffer means that the last write operation
> >     was most likely incomplete
> 
> I think this is probably the best option.

> 
> > 
> >     Also we might rename it from "overflow" to "full" or "filled"
> >     or "sealed" or "complete" that might describe the state more
> >     precisely because there is not a real overflow in some situations
> 
> We could make overflow be m->count > m->size, which would mean that
> m->count == m->size (full) isn't a problem. Although, I would have to
> audit the kernel to see if anything just assumes that m->count is
> always less than m->size.

I am not sure if it is a good idea. I think that having
m->count > m->size is prone to buffer overflow errors in
the buffer reading code. I vote for adding extra flag to
the struct :-)

> This would mean overflow is the correct term, and wouldn't have to
> change it.
> 
> Anyway, that should be a second patch, and we should get my current
> patch (the one to make putc and puts the same) in first, as that
> actually fixes the inconsistency between the two.

Feel free to add

Acked-by: Petr Mladek <pmladek@...e.cz>

for this patch. The question is if it is really really needed in the
final patch set. It might get even partly reverted with the follow
up changes.

Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/