Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d982c172-e368-8917-679e-fc7426b58983@oracle.com>
Date:   Thu, 24 Feb 2022 10:13:32 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
From:   Ross Philipson <ross.philipson@oracle.com>
Subject: Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures
To:     Borislav Petkov <bp@alien8.de>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        daniel.kiper@oracle.com, dpsmith@apertussolutions.com,
        tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com,
        luto@amacapital.net, kanth.ghatraju@oracle.com,
        trenchboot-devel@googlegroups.com
References: <1643303056-22272-1-git-send-email-ross.philipson@oracle.com>
 <1643303056-22272-2-git-send-email-ross.philipson@oracle.com>
 <YgabtFd4Nkpgi+oW@zn.tnic> <2e584fc4-163a-0af4-abe0-9c14996918f2@oracle.com>
 <Ygvy8j9E7WPo6dx0@zn.tnic> <6e5763be-3e9c-0199-24ea-e4f5b226d990@oracle.com>
 <YheLYDdG3w6WGhSM@zn.tnic>
Content-Language: en-US
In-Reply-To: <YheLYDdG3w6WGhSM@zn.tnic>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?V09JUGNyaEZpcWhQMXFwck9MM2FBd0xpM2R2VUNyUUNrWlBBUnhwaStreDlO?=
 =?utf-8?B?K0pSQWZFL1hjOHNxNDlpNnRaZ1pjSjY0TGs0bEpGSzlnNW81LzZVbUs4VktJ?=
 =?utf-8?B?eGRxSjZlZC9SMVBOTUpqc2RmUDF0ZHNSb3g1Sm4yS2MyOURWZmxtTkpVTHM1?=
 =?utf-8?B?QXV3VVpzb0pneUNBaERabGhJS21Za2dvbkowcGVSWFZxTUxWWUduTXJ4WE1T?=
 =?utf-8?B?RFF5QndVTWprNWkwZFFTWXcya3VsRncvMnQxSkZCMDVxUjFmbGRnS2dMWnVB?=
 =?utf-8?B?cVJrSmxPajlGWU5ySGdvSjVyeHZoa1Rkd3JXNVI4RDJHRGJPWXRGR3RnOW1r?=
 =?utf-8?B?UWpEa3NHbktQUTY5eUdHYm4yQmN0aG84OXFFOW5SZDFXdDk3U21tWVlxNzdY?=
 =?utf-8?B?Z3o3K2M0NTlDZmcvZmpTNm96UUNJazRJUWxhTUZvaGovNjJVS2hFYnZwaVM1?=
 =?utf-8?B?MTZzSFQwYlA1YVYyalZPT0hBSnhIQVU1S2hSN0t6K3A2dDBwNktzZGwwSHds?=
 =?utf-8?B?VUh4RkNZMkh6Q1hrajRhd1VCZ1ViUU1vZFdHMCtGUiszN1N6c29YZXVUYzVO?=
 =?utf-8?B?ZTRyTGJ1MmFPR3l2RWQ1eEJKeUpwR0U3Y2FaRFZJaTl6YTFVeHJhOTM4TUdI?=
 =?utf-8?B?Ykc3cTMwOTlkbHZkenZlV01LTFZZZ2VWRER2MVhObUYrWkpBUitFOGpMUnRk?=
 =?utf-8?B?NDlTRmFieThYWHEyRWpvM0krVitZWndlRnM3TzRBL2d2N2ZwOVJKaStyMVcx?=
 =?utf-8?B?L0NOVDdIK0VZY2o2MjFtbzJEcVd5QjFOZlkzNnpEbzJHSmthdXplRzM5MFND?=
 =?utf-8?B?alR1ZjNwazZyYkh3L3R0alZYYTRLcFd3dVFaaldLVzFRdkF6TVQvRHdNOFIy?=
 =?utf-8?B?aXZWVEt6T2M2S01POTM0VmhaRWQ2OEpvMGJMTE9pMTVrS3BVdlFWdXpqSWxX?=
 =?utf-8?B?NW50YXc2eDZNYlUvYXVCamF1TUlDZjF4WG1DRzh0cHZJSmI2VkVzYUhmdXhD?=
 =?utf-8?B?UXV6R3lXOHcrVFV3TFNEK2NqVUtSS3RuQVFrcDhXcDZHMmcvSVhQeHRvekIy?=
 =?utf-8?B?TVFDMUhNYkE2enRsVVc3SVd1K2s5YUhxcXNiOFhrVExQQWtiWndIc2JkdzRp?=
 =?utf-8?B?YnZyVVRyMmZQSTBldWJGQ2hFcE1tNG4wNkJoSjR0R1ZxOWY5Nk05MytxN1Ew?=
 =?utf-8?B?M0E4VmRMdDZ1aDh1NTdLNmhkKytBYW1scVhkeEhWRzM2T1ltTE0vVndEa21p?=
 =?utf-8?B?Z0YrY0JYY3h5OWgzdG1yU1JkN0lZQTNLdTduakF5ZWdodTY1SDNpVXBOQlR1?=
 =?utf-8?B?ekdyTkZTVHJPNi9VMGNDSkFyN2VTUmdJajR4aEhybXBhNklocU14TkVLZHhR?=
 =?utf-8?B?MnVIaGg0REVnbS9TSk5TSmVpU2NTTWVrdllSSzBpMGozRlZlTU1XbC9TYmxw?=
 =?utf-8?B?dE51ZVBxbFVCL0dXQlptOW02MS81NEh0QlZ6bUlSa2krOFdHNDFjNWdFR1Uw?=
 =?utf-8?B?bld6cjV6c2pHaDJqRnpLUk1zV3laWENOZktESHRrMmtkMURGRUdiTGU5ZlBN?=
 =?utf-8?B?bXlTVkFsVURFaW95YUwyK3ozUjVHQ2xqUHorWUFmVDFqY2EwUU5JTHJMRFpy?=
 =?utf-8?B?SFE2RE0wdzkxelNSeFY5a3FTMnE2VUlaKzE4WU9MWW9UUGpvYm8xajdPa2JQ?=
 =?utf-8?B?WWs1SE1EbW9WUlYwcG5lc0FRRnRhcUhwMmtZbkNHL1pOQ2V4ZmZWMGNxbUp1?=
 =?utf-8?B?NitpOWVmNno3aVhhekxNdmtlWGJNTDVHaTBHVStsVVVJTFZkMkhpWFVUQUl6?=
 =?utf-8?B?MHVFTkFoMU16a2UzTjZ1NGNjQ1hwa3ZScWRDN01QYURvbEN4TFE0N0ZhaklB?=
 =?utf-8?B?NXN4U2I4dlZ0a1Y2VzFKZ0FCWTBXc3hEL1JocVdnSnM2RWhVbWMzZ0FBQjdn?=
 =?utf-8?B?RGgvQVh1dy9LcmtNS2ZsOWZVZk5BWEp5L2lsNWRLa0pwTmJpUjlZZUFvZmVZ?=
 =?utf-8?B?azloaFJGWEZ1RVdQVUVYSnhqbUhLZjVkY0hSbFpnejhNUzYzYXp3SE1FNmdq?=
 =?utf-8?B?WUJVYjJTczc2OW9McTc3ZTNucFQyNU1DZVFlMVcyMEVlRy9uT3pGWjZCTzFa?=
 =?utf-8?B?VDJSa0t4Tit3eE9rM1QwelV3U2NCMFV0bWZDbzRxOWduOFRld2dXQmhNVTV3?=
 =?utf-8?B?a0xzWUhSa2R4Nm04K0hSSUlOOXBsUHVZcFk2SzJaY2RkSjlWaFk4bzZMOUJ2?=
 =?utf-8?B?SW1oQXZodGUwb2huUnB4QkVrTU1kMzZYRWkvTWVnT0ZoY256SUlTdFVOT1lM?=
 =?utf-8?Q?mjk6hvyUcUGuaYXMXH?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d03028ca-7690-470f-b95f-08d9f7a839f4
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB3804.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Feb 2022 15:13:35.4166
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: AGDQdYlyW5EJuhZhWSUcMNgk5mqGEjQCraZtlcnssOtAKhhNkV7jB+vv3QAQTlCl9wgw5fUniddbJroKOxq2jLzmdcJC+QYwcuW5+7Dqijc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB2714
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10267 signatures=681306
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 malwarescore=0
 mlxlogscore=999 adultscore=0 bulkscore=0 phishscore=0 suspectscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2201110000 definitions=main-2202240091
X-Proofpoint-GUID: CIVEZFxmaAm_WVM_D4qsRYk-h2fV3yTm
X-Proofpoint-ORIG-GUID: CIVEZFxmaAm_WVM_D4qsRYk-h2fV3yTm
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2/24/22 08:42, Borislav Petkov wrote:
> On Tue, Feb 22, 2022 at 04:01:32PM -0500, Ross Philipson wrote:
>> In the ioremap.c module, the check for NULL is only missing in the
>> functions we updated but the lack of a check was already there before
>> these changes went in.
>>
>> In the setup.c and e820.c modules, the check for NULL is missing in the
>> functions we updated but the lack of a check was already there before
>> these changes went in in those functions. The lack of early_memremap()
>> NULL checks can also be found in other functions in those modules.
> 
> I don't know how to understand this statement: are you saying that,
> because there are other cases where - for whatever reason - the retval
> check is not taking place - you should not do it either?
> 
> Because I can see other places where the return value is checked. I
> mean, if the return value check doesn't matter, why not make this
> function simply void and not bother at all?

Sorry if I am being vague. I will try to clarify.

In the functions I made changes to, some already checked return values
from early_memremap and some didn't prior to my changes. In addition,
there are other places/functions in those files that did not check
return values from early_memremap.

I did a search yesterday and found there are other places where the
return value from early_memremap is not checked in files I did not
touch. In general the handling of return values from early_memremap is
very inconsistent throughout arch/x86.

> 
>> Fixing it in other functions and possibly elsewhere in the arch/x86 code
>> seems to be out of scope for this patch set. We could send separate
>> patches and hunt down other places this check is missing.
> 
> That would be appreciated.

I will submit a follow on patch set that chases down all the places
where early_memremap return values are not checked and fix them. In
addition I will do the same for early_ioremap since it seems to have the
same issues throughout arch/x86.

Thanks
Ross

> 
> Thx.
>