Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20181127031218.24419-2-dan.rue@linaro.org>
References: <20181127031218.24419-1-dan.rue@linaro.org> <20181127031218.24419-2-dan.rue@linaro.org>
From:   Kees Cook <keescook@chromium.org>
Date:   Tue, 27 Nov 2018 09:56:09 -0800
Message-ID: <CAGXu5jJWQX2qwwL4_1wQUWOGU9ZDay8ddZaLLJQJ-1S8h5cgzA@mail.gmail.com>
Subject: Re: [PATCH 1/2] selftests: firmware: remove use of non-standard diff
 -Z option
To:     Dan Rue <dan.rue@linaro.org>
Cc:     LKML <linux-kernel@vger.kernel.org>, Shuah Khan <shuah@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Luis R. Rodriguez" <mcgrof@kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Nov 26, 2018 at 7:12 PM, Dan Rue <dan.rue@linaro.org> wrote:
> diff -Z is used to trim the trailing whitespace when comparing the
> loaded firmware file with the source firmware file. However, per the
> comment in the source code, -Z should not be necessary. In testing, the
> input and output files are identical.
>
> Additionally, -Z is not a standard option and is not available in
> environments such as busybox. When -Z is not supported, diff fails with
> a usage error, which is suppressed, but then causes read_firmwares() to
> exit with a false failure message.
>
> Signed-off-by: Dan Rue <dan.rue@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  tools/testing/selftests/firmware/fw_filesystem.sh | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
> index a4320c4b44dc..466cf2f91ba0 100755
> --- a/tools/testing/selftests/firmware/fw_filesystem.sh
> +++ b/tools/testing/selftests/firmware/fw_filesystem.sh
> @@ -155,11 +155,8 @@ read_firmwares()
>  {
>         for i in $(seq 0 3); do
>                 config_set_read_fw_idx $i
> -               # Verify the contents are what we expect.
> -               # -Z required for now -- check for yourself, md5sum
> -               # on $FW and DIR/read_firmware will yield the same. Even
> -               # cmp agrees, so something is off.
> -               if ! diff -q -Z "$FW" $DIR/read_firmware 2>/dev/null ; then
> +               # Verify the contents match
> +               if ! diff -q "$FW" $DIR/read_firmware 2>/dev/null ; then
>                         echo "request #$i: firmware was not loaded" >&2
>                         exit 1
>                 fi
> @@ -171,7 +168,7 @@ read_firmwares_expect_nofile()
>         for i in $(seq 0 3); do
>                 config_set_read_fw_idx $i
>                 # Ensures contents differ
> -               if diff -q -Z "$FW" $DIR/read_firmware 2>/dev/null ; then
> +               if diff -q "$FW" $DIR/read_firmware 2>/dev/null ; then
>                         echo "request $i: file was not expected to match" >&2
>                         exit 1
>                 fi
> --
> 2.19.1
>


-- 
Kees Cook