Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20231102183751.47413-1-pstanner@redhat.com> <CAKFNMok9WTV5uX3BBfr8eYiuJXvUY6TuYW=tQYvBzdCUYsxcwA@mail.gmail.com>
 <437ec6fa34af1ccfadee2d62770e52d99ebf75c3.camel@redhat.com>
In-Reply-To: <437ec6fa34af1ccfadee2d62770e52d99ebf75c3.camel@redhat.com>
From:   Ryusuke Konishi <konishi.ryusuke@gmail.com>
Date:   Sat, 4 Nov 2023 03:08:48 +0900
Message-ID: <CAKFNMokwjSwzT3T6vQZ_-6evPNUqNUuk+D=LGA5cuOm3-xx0aw@mail.gmail.com>
Subject: Re: [PATCH] fs/nilfs2: copy userspace-array safely
To:     Philipp Stanner <pstanner@redhat.com>
Cc:     linux-nilfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        Dave Airlie <airlied@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Nov 4, 2023 at 2:56=E2=80=AFAM Philipp Stanner wrote:
>
> On Sat, 2023-11-04 at 02:44 +0900, Ryusuke Konishi wrote:
> > On Fri, Nov 3, 2023 at 3:38=E2=80=AFAM 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@redhat.com>
> > > Signed-off-by: Philipp Stanner <pstanner@redhat.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 =E2=80=93 i=
t'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