linux-kernel - Re: [PATCH v2 0/6] scsi: Some 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]

Date:	Thu, 29 Jan 2015 10:16:16 +0100
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Finn Thain <fthain@...egraphics.com.au>
Cc:	"James E.J. Bottomley" <JBottomley@...allels.com>,
	linux-fsdevel@...r.kernel.org, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH v2 0/6] scsi: Some seq_file cleanups/optimizations

On Thu, Jan 29 2015, Finn Thain <fthain@...egraphics.com.au> wrote:

> I have one reservation about this patch series.
>
> For example, the changes,
>
> -	seq_printf(m, "%s", p);
> +	seq_puts(m, p);
>
> These calls are not equivalent because the bounds check is not the same. 
> seq_puts will fail when m->count + strlen(p) == m->size.
>

So will seq_printf:

int seq_vprintf(struct seq_file *m, const char *f, va_list args)
{
	int len;

	if (m->count < m->size) {
		len = vsnprintf(m->buf + m->count, m->size - m->count, f, args);
		if (m->count + len < m->size) {
			m->count += len;
			return 0;
		}
	}
	seq_set_overflow(m);
	return -1;
}

The return value from vsnprintf("%s", p) is by definition the length of
the string p. Yes, vsnprintf may write some of the bytes from the
string to the buffer, but those are effectively discarded if they don't
all fit, since m->count is not updated.

> There's a similar situation with the changes,
>
> -	seq_puts(m, "x");
> +	seq_putc(m, 'x');

It's true that this may cause 'x' to be printed which it might not have
been before. I think this is a bug in seq_puts - it should use <= for
its bounds check. OTOH, seq_printf probably needs to continue using <,
because if the return value is == m->size-m->count, vsnprintf will have
truncated the output, overwriting the last byte with a '\0'.

> Have you considered what the implications might be? Are there any?

I must admit I hadn't thought that deeply about it before, but now it
seems that my patches can only end up utilizing m->buf a bit better
(well, 8 bits, to be precise). If I understand the whole seq_*
interface, overflow will just cause a larger buffer to be allocated and
all the print functions to be called again.

Steven, you've been doing some cleanup in this area, among other things
trying to make all the seq_* functions return void. Could you fill me in
on the status of that?

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