linux-kernel - Re: [PATCH v2 07/11] math64: New DIV_U64_ROUND

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

Date:   Fri, 25 Jun 2021 17:50:08 +0200
From:   Willy Tarreau <w@....eu>
To:     Pali Rohár <pali@...nel.org>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Lunn <andrew@...n.ch>,
        Gregory Clement <gregory.clement@...tlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Vladimir Vid <vladimir.vid@...tura.hr>,
        Marek Behún <kabel@...nel.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 07/11] math64: New DIV_U64_ROUND_CLOSEST helper

On Fri, Jun 25, 2021 at 05:38:03PM +0200, Pali Rohár wrote:
> On Friday 25 June 2021 17:22:31 Geert Uytterhoeven wrote:
> > > +/*
> > > + * DIV_U64_ROUND_CLOSEST - unsigned 64bit divide with 32bit divisor rounded to nearest integer
> > > + * @dividend: unsigned 64bit dividend
> > > + * @divisor: unsigned 32bit divisor
> > > + *
> > > + * Divide unsigned 64bit dividend by unsigned 32bit divisor
> > > + * and round to closest integer.
> > > + *
> > > + * Return: dividend / divisor rounded to nearest integer
> > > + */
> > > +#define DIV_U64_ROUND_CLOSEST(dividend, divisor)       \
> > > +       ({ u32 _tmp = (divisor); div_u64((u64)(dividend) + _tmp / 2, _tmp); })
> > 
> > Given "dividend" should already be an unsigned 64-bit value, I don't
> > think the cast to "u64" is needed. Similar macros in this file also
> > don't have the cast.
> 
> It is just to ensure that plus operation between dividend and _tmp is
> evaluated in 64-bit context to prevent overflow. Just a case when user
> calls this macro with 32-bit dividend param. As it is a macro (and not
> inline function) type is not automatically enforced.

I agree, a large u32 argument added to _tmp/2 could overflow and remain
32 bits, yielding an incorrect result. The cast is mandatory here (and
will either emit no code, or be useful).

The only trap I'm seeing is if a negative signed int is passed in dividend,
it will be sign-extended and will give a large u64 value. A preliminary
u32 cast could avoid this but would break valid u64 arguments, and I'd
claim we never know what the user wants if this happens in the first place.

Willy