Date: Sat, 29 Apr 2017 07:37:20 +0200
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Julius Werner <jwerner@chromium.org>
Cc: linux-kernel@vger.kernel.org,
        Thierry Escande <thierry.escande@collabora.com>,
        Dmitry Torokhov <dtor@chromium.org>,
        Aaron Durbin <adurbin@chromium.org>
Subject: Re: [PATCH v2 2/3] firmware: google: memconsole: Escape unprintable
 characters
Message-ID: <20170429053720.GB11491@kroah.com>
References: <20170428204225.32043-1-jwerner@chromium.org>
 <20170428204225.32043-3-jwerner@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170428204225.32043-3-jwerner@chromium.org>
User-Agent: Mutt/1.8.2 (2017-04-18)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3510
Lines: 91

On Fri, Apr 28, 2017 at 01:42:24PM -0700, Julius Werner wrote:
> Recent improvements in coreboot's memory console allow it to contain
> logs from more than one boot as long as the information persists in
> memory. Since trying to persist a memory buffer across reboots often
> doesn't quite work perfectly it is now more likely for random bit flips
> to occur in the console. With the current implementation this can lead
> to stray control characters that cause weird effects for the most common
> use cases (such as just dumping the console with 'cat').
> 
> This patch changes the memconsole driver to replace unprintable
> characters with '?' by default. It also adds a new /sys/firmware/rawlog
> node next to the existing /sys/firmware/log for use cases where it's
> desired to read the raw characters.

Again, you are doing multiple things here.  Break it up.

And, can userspace handle this change?  You are now changing the format
of the existing file.

> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>  drivers/firmware/google/memconsole.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/google/memconsole.c b/drivers/firmware/google/memconsole.c
> index 166f07c68c02..807fcd4f7f85 100644
> --- a/drivers/firmware/google/memconsole.c
> +++ b/drivers/firmware/google/memconsole.c
> @@ -15,6 +15,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <linux/ctype.h>
>  #include <linux/init.h>
>  #include <linux/sysfs.h>
>  #include <linux/kobject.h>
> @@ -24,7 +25,7 @@
>  
>  static ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
>  
> -static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
> +static ssize_t memconsole_read_raw(struct file *filp, struct kobject *kobp,
>  			       struct bin_attribute *bin_attr, char *buf,
>  			       loff_t pos, size_t count)
>  {
> @@ -33,9 +34,28 @@ static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
>  	return memconsole_read_func(buf, pos, count);
>  }
>  
> -static struct bin_attribute memconsole_bin_attr = {
> +static ssize_t memconsole_read_log(struct file *filp, struct kobject *kobp,
> +			       struct bin_attribute *bin_attr, char *buf,
> +			       loff_t pos, size_t count)
> +{
> +	ssize_t i;
> +	ssize_t rv = memconsole_read_raw(filp, kobp, bin_attr, buf, pos, count);
> +
> +	if (rv > 0)
> +		for (i = 0; i < rv; i++)
> +			if (!isprint(buf[i]) && !isspace(buf[i]))
> +				buf[i] = '?';
> +	return rv;
> +}
> +
> +/* Memconsoles may be much longer than 4K, so need to use binary attributes. */
> +static struct bin_attribute memconsole_log_attr = {
>  	.attr = {.name = "log", .mode = 0444},
> -	.read = memconsole_read,
> +	.read = memconsole_read_log,
> +};
> +static struct bin_attribute memconsole_raw_attr = {
> +	.attr = {.name = "rawlog", .mode = 0444},
> +	.read = memconsole_read_raw,
>  };
>  
>  void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t))
> @@ -46,13 +66,15 @@ EXPORT_SYMBOL(memconsole_setup);
>  
>  int memconsole_sysfs_init(void)
>  {
> -	return sysfs_create_bin_file(firmware_kobj, &memconsole_bin_attr);
> +	return sysfs_create_bin_file(firmware_kobj, &memconsole_log_attr) ||
> +		sysfs_create_bin_file(firmware_kobj, &memconsole_raw_attr);

While it is really rare, please do this properly and create one file,
and then the other, and provide proper error handling here (i.e. if the
second fails, remove the first).

thanks,

greg k-h