Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <0e261e3b-af75-0227-dbc9-5daff71fd020@intel.com>
Date:   Thu, 5 May 2022 11:06:51 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.8.1
Subject: Re: [PATCH v2] test_firmware: fix end of loop test in
 upload_read_show()
Content-Language: en-US
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC:     Tianfei zhang <tianfei.zhang@intel.com>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Lee Jones <lee.jones@linaro.org>,
        Shawn Guo <shawn.guo@linaro.org>,
        <linux-kernel@vger.kernel.org>, <kernel-janitors@vger.kernel.org>
References: <YnPMt/syJAqqOBh3@kili>
From:   Russ Weight <russell.h.weight@intel.com>
In-Reply-To: <YnPMt/syJAqqOBh3@kili>
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?MnlmL3lMQUdteC9RTFQxTTBZTFI0bUloNVFIZkNaQ2plUnV0TFdWRDNWUDRY?=
 =?utf-8?B?eGtka1NGNDczQmdZb3A0UTU3eUduV3JwQ2xUa1NWSkxhcTFneFBXd0pRbmdZ?=
 =?utf-8?B?dlN0cUd0R0xMV09wTlhSbmNGMGdTNjNwcXhETXNBaEd2bnNYWHlRKzBnNzU2?=
 =?utf-8?B?RzdFMXkrTmxwLzRjNWJmOEEyblVHVUJJSFZqVk4wb0E0SlppVnRLdWdSSENJ?=
 =?utf-8?B?MGdzcFlSNmorQUY1Wk4yc3dFUlRVZzRjRXJodTNtSlpoTm5LYzA2dEZ3dHpP?=
 =?utf-8?B?YWJNZysyNFlUOW5GQVp2SGlVNWtxdU96RlRHMWtvOXZYN3pNakloRHR1c3lJ?=
 =?utf-8?B?aXBQNnlralBvYnQwVDUrT1g5Tk82YXk0UHl0TmZBd0EzS1A5ZWdVM1cwdEdk?=
 =?utf-8?B?Zmxud1drU1NrNTFwN1ZoM1JmYnc0VTlQeGVET0R1a0w3Yis1Q1BxOTlQa0Qx?=
 =?utf-8?B?d1pKZ3ZmOTdJVFRUa2JRUWNKSHJ3M2ZBdEZhMHU5ZjM1eUorL3p3alpXMlly?=
 =?utf-8?B?YVd2ZHExWEx2R2pHRzJsK2prbFI3d09kMmE1VjJvdEhvWWx1OG9TZnUwaG8w?=
 =?utf-8?B?ajRzcXVZZUtFUjA4Z2JpUlV5bkdTNDJGVzlrRlUwRW1RcmRXckNJem9aUkxG?=
 =?utf-8?B?WmE4ZWtLYjdGTlljeWRZYzlBaFE2WmQ3dEdnbEhkNnR4MmNIWHNoVU5HWjR6?=
 =?utf-8?B?eFhuYmtEeXBTU1NRbnp6algyM3M3bEVMNEd1TkRXd0NQSC9VM1ZSekd5QU1r?=
 =?utf-8?B?SG95Q1d6c21idEl5SUtUdFZtTmEvVFQ0QVk3b3RDblJtNG9mdno3RVVFT004?=
 =?utf-8?B?Y0RQVlF3V0Y3ZnV6eG1YbTNyZTVCTDVYcGFVamViL0pVNklGK29tL1VkemYr?=
 =?utf-8?B?OVdwZWhoSElVU21YOTJwU09kUnNhZ3NuSGVWdTV6OFBHbndub2UzSDFnSzZM?=
 =?utf-8?B?bi9tVUdWcjM4VUJncW8xcGpUMytaTHQxNC9WRHFiTXFSRmluWWJsUG1TczE5?=
 =?utf-8?B?WVg5cE1seDIxSXBSdDJDU0VOTG5zOHZCV2ZlTE5BOFdPa05OOTBaRm1JbmVW?=
 =?utf-8?B?OUxrRFpPSXpBYjkwejU0UWZkMDcyVXBPU1FndGVRZzI5VzgxN2N3UmpiODcz?=
 =?utf-8?B?c1kwZVl1cDFYU0t1SFZ4UlJQejFWYmZlT2V1Y2k1NW5EUXdnS3hTVzF5L0Fz?=
 =?utf-8?B?VHFDdW9IYnljMEVRRUhlQ2ZQYmI0VTFFUVNCTlFpcDg2SEVuRDgzT1JQUnND?=
 =?utf-8?B?d1FVZ0NaVGFEY090T0xvQXFnZ3p4OC9qamU1eXdmK3dFdU9EU2diNnUvdk9O?=
 =?utf-8?B?QVF3a0duaEF2ZTZOTnB1d3RtTVE0cVVvZFpGdjhtVE41NWJTNXFycWhwN2RM?=
 =?utf-8?B?dkFuVGpXUlBnZUVaeC9SMHIvRCs0aW5XNDIreFdqbUZsd25QMGhqcHkrWU81?=
 =?utf-8?B?NWkrK1NybTNaQmJnU29jUml5QXdra2NHblorVXR0UHZlUnBiaDJDeXVQcnQx?=
 =?utf-8?B?SXdhNjg2dm5zczRyWU5CNW9XT3ZsNTVKL2xHcXJjd3ZKU2F1UTRQelFsRDV5?=
 =?utf-8?B?ZGxpMWNrL2tsWlR5dnpZQmVEclB5SEw2amhwSGhMbEp2YWYra3MyNUxFVkow?=
 =?utf-8?B?SUhqY1BneFdkQTBWK3lwUmg4a0krR0dBeUlZN2JyTjBnY0twY2xOd2tjTC9Y?=
 =?utf-8?B?c0lKTUljWmhnd1RDb1ZuajhFYnErd0xybEhxZFZta2RWbDBpb1AwSDhoTXRB?=
 =?utf-8?B?OWhvMkJBQ3J1cW1ZWWVNZWV1c0YvWEFWR2QrRHZBa2h3aldvN1h2cUs5TC83?=
 =?utf-8?B?Z29ZV2VrTS9GeVNTYUMrV3c4NWFHQ2xReHdpK0VoN0RPSFJVTktpbjNseGRI?=
 =?utf-8?B?Mmd4VUFST2pNbnBablZkaHloZFNmQmErVkJURVBMc0swZDhjR0ovQitsY0FK?=
 =?utf-8?B?MlhENlFDZW0zczBGNElyRzh0ZVk5ZlVCOEx6ekUxaXBEQ2pvOWRsSERJMlpD?=
 =?utf-8?B?L3llb0Yvbk9KZFpjbEsvV1FyYmZsTitUNnZUY3Jjb05DQ3FGVXdIQ09oM1Ix?=
 =?utf-8?B?dXNrcGRldGdsU00xK05NNnhhTXZsMDBCSjM4TG1tRVQwYVp5VVdaaTBjUUN6?=
 =?utf-8?B?ZFYva3FzTVY5Sm9SeVVJTGx4dWM3bVJBT0ZlNnphcW1PUnh6aEtFQUFyMkhm?=
 =?utf-8?B?NEcvdkxyR3VaS0ZWSHRrR2xIVks4Q3MxeW9SRzIxTkI1V1JsNEJPY2pCWjdJ?=
 =?utf-8?B?aG9STHJGVitpY2t1eTNyNUt2SVBhazdXRGhKZmE4aU5vcUFQbmVIZ25MbzRy?=
 =?utf-8?B?UG1rMjZRbWpkb0RNQUNrRFo4U1dQQzBrd1F1dnoyQndSK1NJM1djUkR1Sy9p?=
 =?utf-8?Q?JXRfUBuwbjXXE8NE=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b9509ead-8f72-4557-1747-08da2ec20a75
X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1899.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2022 18:06:56.2044
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: YB0hjqbytK4Mw74vB0REuk/tSbcwfV4VPPFm+1grUixyjupmKGMklm6HmPHTLYtMJhrPOrLSJ1kg1lNUaKlvn2nkgkzOycjyEGVUUvHJg8E=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6168
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable 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 5/5/22 06:10, Dan Carpenter wrote:
> If a list_for_each_entry() loop exits without hitting a break statement
> then the iterator points to invalid memory.  So in this code the
> "tst->name" dereference is an out bounds read.  It's an offset from the
> &test_upload_list pointer and it will likely work fine most of the time
> but it's not correct.
>
> One alternative is to fix this this by changing the test to:
>
> 	if (list_entry_is_head(tst, &test_upload_list, node)) {
>
> But the simpler, trendy new way is just create a new variable and test
> for NULL.
>
> Fixes: a31ad463b72d ("test_firmware: Add test support for firmware upload")
Reviewed-by: Russ Weight <russell.h.weight@intel.com>

Thanks,
- Russ

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  lib/test_firmware.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lib/test_firmware.c b/lib/test_firmware.c
> index 76115c1a2629..c82b65947ce6 100644
> --- a/lib/test_firmware.c
> +++ b/lib/test_firmware.c
> @@ -1392,7 +1392,8 @@ static ssize_t upload_read_show(struct device *dev,
>  				struct device_attribute *attr,
>  				char *buf)
>  {
> -	struct test_firmware_upload *tst;
> +	struct test_firmware_upload *tst = NULL;
> +	struct test_firmware_upload *tst_iter;
>  	int ret = -EINVAL;
>  
>  	if (!test_fw_config->upload_name) {
> @@ -1401,11 +1402,13 @@ static ssize_t upload_read_show(struct device *dev,
>  	}
>  
>  	mutex_lock(&test_fw_mutex);
> -	list_for_each_entry(tst, &test_upload_list, node)
> -		if (tst->name == test_fw_config->upload_name)
> +	list_for_each_entry(tst_iter, &test_upload_list, node)
> +		if (tst_iter->name == test_fw_config->upload_name) {
> +			tst = tst_iter;
>  			break;
> +		}
>  
> -	if (tst->name != test_fw_config->upload_name) {
> +	if (!tst) {
>  		pr_err("Firmware name not found: %s\n",
>  		       test_fw_config->upload_name);
>  		goto out;