linux-kernel - Re: [PATCH 2/4] kexec: remove compat_sys_kexec

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]

Date:   Sat, 26 Sep 2020 23:10:05 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Eric Biederman <ebiederm@...ssion.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>, kexec@...ts.infradead.org
Subject: Re: [PATCH 2/4] kexec: remove compat_sys_kexec_load syscall

On Sat, Sep 19, 2020 at 7:37 AM Christoph Hellwig <hch@...radead.org> wrote:

> > +             struct compat_kexec_segment __user *cs = (void __user *)segments;
> > +             struct compat_kexec_segment segment;
> > +             int i;
> > +             for (i=0; i< nr_segments; i++) {
>
> Missing empty line after the variable declarations and really strange
> indentation.
>
> > +                     copy_from_user(&segment, &cs[i], sizeof(segment));
>
> Missing return value check.
>
> > +                     if (ret)
> > +                             break;
> > +
> > +                     image->segment[i] = (struct kexec_segment) {
> > +                             .buf   = compat_ptr(segment.buf),
> > +                             .bufsz = segment.bufsz,
> > +                             .mem   = segment.mem,
> > +                             .memsz = segment.memsz,
> > +                     };
> > +             }
>
> I'd split the whole compat handling into a helper, and I'd probably
> use the unsafe_get/put user to optimize it a little more.
>
> > +     } else {
> > +             ret = copy_from_user(image->segment, segments, segment_bytes);
> > +     }
> >       if (ret)
> >               ret = -EFAULT;
>
> Why not just
>
>                 if (copy_from_user(image->segment, segments, segment_bytes))
>                         ret = -EFAULT;
>
> ?

Addressed all of these now, thanks for the suggestions!

I had already fixed the missing error handling after the kbuild bot
pointed that out. The separate function does improve the error
handling.

I ended up not using unsafe_get/put since I find the copy_from_user
based loop more readable and it should lead to smaller object code in
most cases as well. kexec is not performance critical, so readability
seems more important here.

      Arnd