Re: [Xen-devel] [PATCH 7/8] x86/setup: simplify handling of initrdidx when no initrd present

From: Jan Beulich <jbeulich@suse.com>
To: Julien Grall <julien@xen.org>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Varad Gautam" <vrd@amazon.de>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>,
	"Hongyan Xia" <hongyxia@amazon.com>,
	xen-devel@lists.xenproject.org,
	"Paul Durrant" <pdurrant@amazon.co.uk>,
	"David Woodhouse" <dwmw2@infradead.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 7/8] x86/setup: simplify handling of initrdidx when no initrd present
Date: Tue, 25 Feb 2020 13:34:51 +0100	[thread overview]
Message-ID: <44f074ee-b47c-0c20-02d8-8bee1557e503@suse.com> (raw)
In-Reply-To: <519b73bb-2db3-75e4-db81-3781c462290e@xen.org>

On 24.02.2020 14:31, Julien Grall wrote:
> On 21/02/2020 16:59, Jan Beulich wrote:
>> On 01.02.2020 01:33, David Woodhouse wrote:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> Remove a ternary operator that made my brain hurt.
>>
>> Personally I'd prefer the code to stay as is, but if Andrew agrees
>> with this being an improvement, then I also wouldn't want to stand
>> in the way. If it is to go in I have a few small adjustment requests:
>>
>>> Replace it with something simpler that makes it somewhat clearer that
>>> the check for initrdidx < mbi->mods_count is because mbi->mods_count
>>> is what find_first_bit() will return when it doesn't find anything.
>>
>> Especially in light of the recent XSA-307 I'd like to ask that we
>> avoid imprecise statements like this: Afaict find_first_bit() may
>> also validly return any value larger than the passed in bitmap
>> length.
> 
> Is it? I though that all the callers are now returning 'size' in all the 
> error cases.

Taking (part of) the x86 example:

>#define find_next_bit(addr, size, off) ({                                   \
>    unsigned int r__;                                                       \
>    const unsigned long *a__ = (addr);                                      \
>    unsigned int s__ = (size);                                              \
>    unsigned int o__ = (off);                                               \
>    if ( o__ >= s__ )                                                       \
>        r__ = s__;                                                          \

This is what did get adjusted, guaranteeing "size" to be returned for
too large an "offset".

>    else if ( __builtin_constant_p(size) && s__ <= BITS_PER_LONG )          \
>        r__ = o__ + __scanbit(*(const unsigned long *)(a__) >> o__, s__);   \

Yet this was (deliberately) left untouched. Without s__ getting
replaced by s__ - o__ it may still produce a value larger than
"size", though.

Further, even if all current implementations obeyed by the more
strict rule, this still wouldn't mean callers are actually permitted
to assume such. The more strict rule would then also need to be
written down, such that it won't get violated again in the future
(by changes to an existing arch's implementation, or by a new port).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel