linux-kernel - Re: [PATCH 0/5] firmware: Add support for loading compressed files

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]

Date:   Mon, 20 May 2019 11:26:59 -0600
From:   shuah <shuah@...nel.org>
To:     Takashi Iwai <tiwai@...e.de>, Kees Cook <keescook@...omium.org>,
        shuah <shuah@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/5] firmware: Add support for loading compressed files

On 5/20/19 10:22 AM, Takashi Iwai wrote:
> On Mon, 20 May 2019 17:18:48 +0200,
> Takashi Iwai wrote:
>>
>> On Mon, 20 May 2019 16:39:37 +0200,
>> Takashi Iwai wrote:
>>>
>>> On Mon, 20 May 2019 11:39:29 +0200,
>>> Greg Kroah-Hartman wrote:
>>>>
>>>> On Mon, May 20, 2019 at 11:26:42AM +0200, Takashi Iwai wrote:
>>>>> Hi,
>>>>>
>>>>> this is a patch set to add the support for loading compressed firmware
>>>>> files.
>>>>>
>>>>> The primary motivation is to reduce the storage size; e.g. currently
>>>>> the amount of /lib/firmware on my machine counts up to 419MB, and this
>>>>> can be reduced to 130MB file compression.  No bad deal.
>>>>>
>>>>> The feature adds only fallback to the compressed file, so it should
>>>>> work as it was as long as the normal firmware file is present.  The
>>>>> f/w loader decompresses the content, so that there is no change needed
>>>>> in the caller side.
>>>>>
>>>>> Currently only XZ format is supported.  A caveat is that the kernel XZ
>>>>> helper code supports only CRC32 (or none) integrity check type, so
>>>>> you'll have to compress the files via xz -C crc32 option.
>>>>>
>>>>> The patch set begins with a few other improvements and refactoring,
>>>>> followed by the compression support.
>>>>>
>>>>> In addition to this, dracut needs a small fix to deal with the *.xz
>>>>> files.
>>>>>
>>>>> Also, the latest patchset is found in topic/fw-decompress branch of my
>>>>> sound.git tree:
>>>>>    git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
>>>>
>>>> After a quick review, these all look good to me, nice job.
>>>>
>>>> One recommendation, can we add support for testing this to the
>>>> tools/testing/selftests/firmware/ tests?  And you did run those
>>>> regression tests to verify that you didn't get any of the config options
>>>> messed up, right? :)
>>>
>>> Now I've been testing the firmware selftest, and this turned out to be
>>> surprisingly difficult on my system.  By some reason, the test always
>>> fails at the point triggering the request (line 58 of
>>> fw_filesystem.sh):
>>>
>>>    if ! echo -n "$NAME" >"$DIR"/trigger_request ; then
>>> 	....
>>>
>>> Judging from the strace output, this echo writes only the first byte
>>> of $NAME.  Then kernfs write op is invoked and it deals this one byte
>>> input as if a whole argument were passed, leading to an error.
>>>
>>> My temporary workaround was to replace the all "echo" call with
>>> "/usr/bin/echo".
>>>
>>> Then it hits a similar write error at the places like:
>>>
>>> 	echo 1 > $DIR/config_sync_direct
>>>
>>> This could be worked around by adding -n option to echo.
>>>
>>> Finally, I noticed that the user-fallback doesn't work on my system
>>> any longer and the test stopped.  This is expected, so it implies that
>>> all direct loading tests passed.
>>>
>>> FWIW, my system is openSUSE Leap 15.1.  Does anyone experience a
>>> similar problem?
>>
>> This seems to be a regression on 5.2-rc1.
>> The tests on 4.20 worked fine.  5.1 worked, but gave the error at
>> fallback test instead of skipping.  This is likely another regression,
>> but irrelevant with the major issue as above.
>>
>> Now bisecting...
> 
> Still in bisection, but it's timeout, I'll have to leave now...
> 
> FWIW, the regression seems to have been introduced around the latest
> kselftest merge.
> The commit 4c7b63a32d54 is bad, and the commit 9cbda1bddb4c good.
> 
> Shuah, could you check it?
> 
> 

Kees,

Could this be related to your selftest Makefile test run output
refactoring work?

I did a quick test on 5.2 after my first kselftest update without
the refactor work - firmware test worked.

I am thinking this is related to

eff3ee303d0d
selftests: Extract single-test shell logic from lib.mk

bash vs. sh difference in "echo" command behavior is the cause
is my best guess. Will you be able to take a look at this?

sudo make -C tools/testing/selftests/firmware/ run_tests

will show the difference.

thanks,
-- Shuah