linux-kernel - Re: [PATCH v2 1/2] tools/bootconfig: Align the bootconfig applied initrd image size to 4

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: <CAHk-=wj-eXNoAXmtsr3hiLWsJffVpi9sOzy6W-gQwqAqbDBv5A@mail.gmail.com>
Date:   Mon, 16 Nov 2020 10:29:45 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Chen Yu <yu.c.chen@...el.com>,
        Chen Yu <yu.chen.surf@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH v2 1/2] tools/bootconfig: Align the bootconfig applied
 initrd image size to 4

On Mon, Nov 16, 2020 at 8:22 AM Masami Hiramatsu <mhiramat@...nel.org> wrote:
>
>         /* TODO: Ensure the @path is initramfs/initrd image */
> -       ret = write(fd, data, size + 8);
> +       ret = write(fd, data, size);
>         if (ret < 0) {
>                 pr_err("Failed to apply a boot config: %d\n", ret);

Side note: that's not the right error check for a write() call.

The problem predates your patch, so it's independent, I just noticed
as I looked at it.

The right error check is

        if (ret != size) {

because partial writes can happen even with regular files (yeah, it's
not POSIX, but NFS is a thing).

And the filesystem filling up is a possibility too, of course.

> +               /* Write padding null characters */
> +               ret = write(fd, padbuf, pad);
> +               if (ret < 0) {
...
> +       ret = write(fd, &size, sizeof(u32));
> +       ret = write(fd, &csum, sizeof(u32));

Those two guys don't check at all...

>         /* Write a magic word of the bootconfig */
>         ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
>         if (ret < 0) {

.. and same comment.

Anyway, I'm not sure this is worth worrying about - even on NFS the
partial IO thing is basically non-existent.

But the filesystem full (or file size limit, or whatever) case is real
even on POSIX filesystems, and testing for write errors by checking
for a negative return is simply incorrect.

Partial writes are an issue (although the _next_ write will then
return an error, so you have to be unlucky to hit that partial write
on the very last write() that you do and that you test the return
value incorrectly).

The fact that it works in all normal circumstances just means that
these kinds of bugs tend to stay around, until somebody is really
unlocky and then you have inexplicably corrupt files..

                 Linus