linux-kernel - Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota

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]

Message-ID: <20150704133237.GB17917@localhost.localdomain>
Date:	Sat, 4 Jul 2015 16:32:37 +0300
From:	Sergei Zviagintsev <sergei@...v.net>
To:	David Herrmann <dh.herrmann@...il.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Daniel Mack <daniel@...que.org>,
	David Herrmann <dh.herrmann@...glemail.com>,
	Djalal Harouni <tixxdz@...ndz.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC 5/5] kdbus: improve tests on incrementing quota

Hi,

On Sat, Jul 04, 2015 at 01:42:31PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Jul 2, 2015 at 3:45 PM, Sergei Zviagintsev <sergei@...v.net> wrote:
> > Hi David,
> >
> > Thank you for reviewing and providing comments on these all! I answered below.
> >
> > On Thu, Jul 02, 2015 at 10:50:47AM +0200, David Herrmann wrote:
> >> Hi
> >>
> >> On Sun, Jun 28, 2015 at 3:17 PM, Sergei Zviagintsev <sergei@...v.net> wrote:
> >> >  1) Rewrite
> >> >
> >> >         quota->memory + memory > U32_MAX
> >> >
> >> >     as
> >> >         U32_MAX - quota->memory < memory
> >> >
> >> >     and provide the comment on why we need that check.
> >> >
> >> >     We have no overflow issue in the original expression when size_t is
> >> >     32-bit because the previous one (available - quota->memory < memory)
> >> >     guarantees that quota->memory + memory doesn't exceed `available'
> >> >     which is <= U32_MAX in that case.
> >> >
> >> >     But lets stay explicit rather than implicit, it would save us from
> >> >     describing HOW the code works.
> >> >
> >> >  2) Add WARN_ON when quota->msgs > KDBUS_CONN_MAX_MSGS
> >> >
> >> >     This is somewhat inconsistent, so we need to properly report it.
> >>
> >> I don't see the purpose of this WARN_ON(). Sure, ">" should never
> >> happen, but that doesn't mean we have to add a WARN_ON. I'd just keep
> >> the code as it is.
> >
> > I agree on WARN_ON. The intention of this change was to provide
> > consistency. Current code checks for 'quota->msgs > KDBUS_CONN_MAX_MSGS'
> > having '>=' test. If this ever happens, it means that we have a bug, but
> > silently ignore it.
> >
> > If we agree that '>' case should never happen, isn't it better to
> > place '==' instead of '>=' in the original test?
> 
> I don't see why. This code does not care whether quota->msgs is bigger
> than MAX_MSGS. Sure, it does not happen in current code, but this
> code-path really doesn't care whether that case can happen or not. All
> it does, it verify that it is smaller. Hence, we use ">=".
> 
> Furthermore, I usually prefer being rather safe than sorry. WARN_ON()s
> are usually not free, but ">=" is for free, if we already have a
> condition.

ok, thank you for explanation!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/