Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [greybus-dev] [PATCH] greybus: remove stray nul byte in
 apb_log_enable_read output
To:     Alex Elder <elder@linaro.org>, Johan Hovold <johan@kernel.org>,
        Alex Elder <elder@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     greybus-dev@lists.linaro.org, linux-kernel@vger.kernel.org
References: <20210326152254.733066-1-linux@rasmusvillemoes.dk>
 <3a04ad94-f8c3-4d2e-c6a2-d498ac3cbeb0@linaro.org>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <bca9a507-0cfb-936c-5fce-a5fa3f05b0cd@rasmusvillemoes.dk>
Date:   Fri, 26 Mar 2021 18:05:47 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <3a04ad94-f8c3-4d2e-c6a2-d498ac3cbeb0@linaro.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 26/03/2021 17.31, Alex Elder wrote:
> On 3/26/21 10:22 AM, Rasmus Villemoes wrote:
>> Including a nul byte in the otherwise human-readable ascii output
>> from this debugfs file is probably not intended.
> 
> Looking only at the comments above simple_read_from_buffer(),
> the last argument is the size of the buffer (tmp_buf in this
> case).  So "3" is proper.

The size of the buffer is 3 because that's what sprintf() is gonna need
to print one digit, '\n' and a nul byte. That doesn't necessarily imply
that the entire buffer is meant to be sent to userspace.

> But looking at callers, it seems that the trailing NUL is
> often excluded this way.
> 
> I don't really have a problem with your patch, but could
> you explain why having the NUL byte included is an actual
> problem?  A short statement about that would provide better
> context than just "probably not intended."

debugfs files are AFAIK intended to be used with simple "cat foo", "echo
1 > foo" in shell (scripts and interactive). Having non-printable
characters returned from that "cat foo" is odd (and can sometimes break
scripts that e.g. "grep 1 foo/*/*/bar" when grep stops because it thinks
one of the files is binary, or when the output of that is further piped
somewhere).

At the very least, it's inconsistent for this one, those in
greybus/svc.c do just return the ascii digits and the newline (and if
one followed your argument above and let those pass 16 instead of desc
one would leak a few bytes of uninitialized kernel stack to userspace).

I said "probably not intended" because for all I know, it might be
intentional. I just doubt it.

Rasmus