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@xxxxxxxxxx>
> > > Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
> > > ---
> > > 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
