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] [day] [month] [year] [list]

Message-Id: <20201117090936.cfa520bc662954d2fb6fc895@kernel.org>
Date:   Tue, 17 Nov 2020 09:09:36 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.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, 16 Nov 2020 10:29:45 -0800
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> 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.

Yes, and I also found the error message used return code, but it must use
errno...  

> 
> > +               /* 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...

Oops, OK, fix it.

> 
> >         /* 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..

Agreed. If I hit the partial write, I need to roll it back. Or bootconfig
command can not find the magic word and initrd image may just increase
the size with the garbage data.

Thank you!

-- 
Masami Hiramatsu <mhiramat@...nel.org>