linux-kernel - Re: [PATCH printk v3 6/6] printk: syslog: close window between wait and read

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: <YNneRIEIa7pHahTu@alley>
Date:   Mon, 28 Jun 2021 16:35:48 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     John Ogness <john.ogness@...utronix.de>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH printk v3 6/6] printk: syslog: close window between wait
 and read

On Fri 2021-06-25 09:33:54, Steven Rostedt wrote:
> On Thu, 24 Jun 2021 13:17:48 +0206
> John Ogness <john.ogness@...utronix.de> wrote:
> 
> > +	 * @syslog_lock is held when entering the read loop to prevent
> > +	 * another reader from modifying @syslog_seq.
> 
> You should add to the above comment:
> 
> 	 * And the @syslog_lock is released before exiting the loop.
> 
> Because it's not normal to enter a loop locked, and have it unlocked
> when exiting the loop. And I can envision in the future, someone might
> add a break (for error) while still holding the lock.

I was double checking the code and the locking is really hard to
follow. I would if the following approach make it easier. The main
trick is that the lock is taken at the beginnig and release at
the end. It is only temporary released around a single line
when needed.

static int syslog_print(char __user *buf, int size)
{
	struct printk_info info;
	struct printk_record r;
	char *text;
	int len = 0;
	u64 seq;

	text = kmalloc(CONSOLE_LOG_MAX, GFP_KERNEL);
	if (!text)
		return -ENOMEM;

	prb_rec_init_rd(&r, &info, text, CONSOLE_LOG_MAX);

	mutex_lock(&syslog_lock);

	/*
	 * Wait for the @syslog_seq record to be vailable. @syslog_seq may
	 * change while waiting.
	 */
	do {
		seq = syslog_seq;

		mutex_unlock(&syslog_lock);
		len = wait_event_interruptible(log_wait, prb_read_valid(prb, seq, NULL));
		mutex_lock(&syslog_lock);

		if (len)
			goto out;
	} while (syslog_seq != seq);

	/*
	 * Copy records that fit into the buffer. The above cycle makes sure
	 * that the first record is always available.
	 */
	do {
		size_t n;
		size_t skip;
		unsigned long err;

		if (!prb_read_valid(prb, syslog_seq, &r))
			break;

		if (r.info->seq != syslog_seq) {
			/* message is gone, move to next valid one */
			syslog_seq = r.info->seq;
			syslog_partial = 0;
		}

		/*
		 * To keep reading/counting partial line consistent,
		 * use printk_time value as of the beginning of a line.
		 */
		if (!syslog_partial)
			syslog_time = printk_time;

		skip = syslog_partial;
		n = record_print_text(&r, true, syslog_time);
		if (n - syslog_partial <= size) {
			/* message fits into buffer, move forward */
			syslog_seq = r.info->seq + 1;
			n -= syslog_partial;
			syslog_partial = 0;
		} else if (!len){
			/* partial read(), remember position */
			n = size;
			syslog_partial += n;
		} else
			n = 0;

		if (!n)
			break;

		mutex_unlock(&syslog_lock);
		err = copy_to_user(buf, text + skip, n);
		mutex_lock(&syslog_lock);

		if (err && !len) {
			len = -EFAULT;
			break;
		}

		len += n;
		size -= n;
		buf += n;
	} while(size);
out:
	mutex_unlock(&syslog_lock);
	kfree(text);
	return len;
}

Best Regards,
Petr