linux-kernel - Re: [PATCH] fs/nilfs2: copy userspace-array safely

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: <CAKFNMokwjSwzT3T6vQZ_-6evPNUqNUuk+D=LGA5cuOm3-xx0aw@mail.gmail.com>
Date:   Sat, 4 Nov 2023 03:08:48 +0900
From:   Ryusuke Konishi <konishi.ryusuke@...il.com>
To:     Philipp Stanner <pstanner@...hat.com>
Cc:     linux-nilfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        Dave Airlie <airlied@...hat.com>
Subject: Re: [PATCH] fs/nilfs2: copy userspace-array safely

On Sat, Nov 4, 2023 at 2:56 AM Philipp Stanner wrote:
>
> On Sat, 2023-11-04 at 02:44 +0900, Ryusuke Konishi wrote:
> > On Fri, Nov 3, 2023 at 3:38 AM Philipp Stanner wrote:
> > >
> > > ioctl.c utilizes memdup_user() to copy a userspace array. This is
> > > done
> > > without an overflow-check.
> > >
> > > Use the new wrapper memdup_array_user() to copy the array more
> > > safely.
> > >
> > > Suggested-by: Dave Airlie <airlied@...hat.com>
> > > Signed-off-by: Philipp Stanner <pstanner@...hat.com>
> > > ---
> > > Linus recently merged this new wrapper for Kernel v6.7
> >
> > The following overflow check is performed just before the usage of
> > memdup_user():
> >
> >         if (nsegs > UINT_MAX / sizeof(__u64))
> >                 goto out;
> >
> > This was introduced by commit 1ecd3c7ea76488 ("nilfs2: avoid
> > overflowing segment numbers in nilfs_ioctl_clean_segments()") to
> > avoid
> > overflowing nsegs * sizeof(__u64) in the subsequent call to
> > memdup_user().
> >
> > I learned about memdup_array_user() this time, and it seems to check
> > for overflow when multiplying two size_t arguments (i.e. the number
> > of
> > elements and size of the array to be copied).
> >
> > Since size_t is 32-bit or 64-bit depending on the architecture, I
> > think the overflow check that memdup_array_user() does
> > is included in the above upper limit check by UINT_MAX.
> >
> > So, for security reasons, I don't think this change is necessary.
> > (Am
> > I missing something?)
>
> No, I think you are right. My commit message was very generic – it's
> more about unifying array-duplication.
> I should rephrase it.
>
> >
> > In terms of cleanup, I think the clarification this patch brings is
> > good, but in that case, I'm concerned about the duplication of
> > overflow checks.
>
> Alright, so would you prefer a patch that uses memdup_array_user() and,
> consequently, removes the preceding check?
>
> Regards,
> P.

Yeah.  If you could revise it as a cleanup patch, I would like to
adopt it for the next cycle.

Regards,
Ryusuke Konishi