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: <87zgvetlc3.fsf@jogness.linutronix.de>
Date:   Fri, 25 Jun 2021 10:17:40 +0206
From:   John Ogness <john.ogness@...utronix.de>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.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 2021-06-24, Petr Mladek <pmladek@...e.com> wrote:
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index 90954cb5a0ab..4737804d6c6d 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -1542,8 +1570,13 @@ static int syslog_print(char __user *buf, int size)
>>  		len += n;
>>  		size -= n;
>>  		buf += n;
>> -	}
>>  
>> +		if (!size)
>> +			break;
>
> This looks like an unrelated optimization. If I get it correctly, it
> does not change the existing behavior.

It was a necessary change in order to preserve the existing logic but
allow the lock to be held when enterring the loop. Before the patch we
have:

        ...get seq to read...

        while (size > 0) {
            mutex_lock(&syslog_lock);
            ...read record...
            mutex_unlock(&syslog_lock);
            ...copy record...
       }

After the patch we enter the loop with the lock already held. So this
changes the code to:

        mutex_lock(&syslog_lock);
        ...get seq to read...

        for (;;) {
            ...read record...
            mutex_unlock(&syslog_lock);
            ...copy record...

            if (!size)
                break;
            mutex_lock(&syslog_lock);               
        }

Note that @size always starts with >0, so there is no need to check it
at the beginning of the loop. And checking for !0 instead of >0 is also
ok, since @size will never be less than zero.

> The next cycle would end up with n == 0 and break anyway.

Doing an extra loop of reading more data and sprinting it into the
temporary buffer even though we know the user buffer is not desirable.

If you insisted on keeping the "while (size > 0)" loop, then there would
be an unnecessary lock/unlock call and the code gets even more complex.

I could add some comments to the implementation if you prefer.

> The patch itself makes sense. It somehow fixes a long standing race.
> Even though the result still might be racy. The lock is released
> when each record is copied to the user-provided buffer.

I do not understand this conclusion. The existing race is
real. SYSLOG_ACTION_READ could return with no data, not because there is
no records available, but because the race was hit. With this patch that
race is closed: SYSLOG_ACTION_READ will either return with data or with
an error.

You claim the result is still racy, but I do not know what you are
referring to. If you have multiple readers, they will get different
records (and record pieces), but collectively no data would be lost and
no data would be redundant. And no readers would return from
SYSLOG_ACTION_READ without data.

> I would feel more comfortable if we handled the optimization one of
> the suggested way.

There is no optimization here. Perhaps you have missed that the loop
changes from "while (size > 0)" to "for (;;)".

John Ogness