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: <20140929124029.64aa1b0a@gandalf.local.home>
Date:	Mon, 29 Sep 2014 12:40:29 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Petr Mladek <pmladek@...e.cz>
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, 29 Sep 2014 18:23:34 +0200
Petr Mladek <pmladek@...e.cz> wrote:
 
> Do you prefer:
> 
>       + the extra struct member?

I hate the extra member.

>       + or always filling some valid data?

Not needed.

>       + or invalidating the buffer when the overflow is set?

Not needed.

> 
> > > 
> > >   + 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 :-)

Reading the seq_file code, it is really an all or nothing approach. It
even does the saving of the m->count and on overflow, will reset it to
what it did originally.

If an overflow happens, the buffer count should either be reset to what
it was (throw away anything that was written), or discarded completely,
which it sometimes does.

I'm going to write up some patches that converts overflow to be count =
size + 1, and then updating everything to be an all or nothing approach.

I'll take Joe's patches to convert them to void as well, with the
addition of a seq_is_full() helper for those places that wont waste time
adding more if it's just going to be discarded.

But first, I'm off to my doctor's appointment followed by more PT.

-- Steve
--
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/