linux-kernel - Re: checkpatch potential false positive

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:   Mon, 6 Nov 2017 08:33:36 +0000
From:   Andy Whitcroft <apw@...onical.com>
To:     "Tobin C. Harding" <me@...in.cc>
Cc:     Joe Perches <joe@...ches.com>, linux-kernel@...r.kernel.org
Subject: Re: checkpatch potential false positive

On Mon, Nov 06, 2017 at 03:19:14PM +1100, Tobin C. Harding wrote:
> Hi,
> 
> When parsing drivers/staging/unisys/visorbus/visorchipset.c in Greg's
> staging tree checkpatch emits
> 
> --------------
> visorchipset.c
> --------------
> WARNING: char * array declaration might be better as static const
> #1050: FILE: visorchipset.c:1050:
> +	char *envp[] = { env_cmd, env_id, env_state, env_bus, env_dev,
> 
> WARNING: char * array declaration might be better as static const
> #1140: FILE: visorchipset.c:1140:
> +	char *envp[] = { env_selftest, NULL };
> 
> total: 0 errors, 2 warnings, 1694 lines checked
> 
> I may be wrong but I think the code in question is clean and
> correct. Since checkpatch is saying this _might_ be better ... perhaps
> checkpatch could emit CHECK instead of WARNING for this?
> 
> Here is the complete function to save finding the file.
> 
> /*
>  * parahotplug_request_kickoff() - initiate parahotplug request
>  * @req: the request to initiate
>  *
>  * Cause uevent to run the user level script to do the disable/enable specified
>  * in the parahotplug_request.
>  */
> static int parahotplug_request_kickoff(struct parahotplug_request *req)
> {
> 	struct controlvm_message_packet *cmd = &req->msg.cmd;
> 	char env_cmd[40], env_id[40], env_state[40], env_bus[40], env_dev[40],
> 	     env_func[40];
> 	char *envp[] = { env_cmd, env_id, env_state, env_bus, env_dev,
> 			 env_func, NULL
> 	};
> 
> 	sprintf(env_cmd, "VISOR_PARAHOTPLUG=1");
> 	sprintf(env_id, "VISOR_PARAHOTPLUG_ID=%d", req->id);
> 	sprintf(env_state, "VISOR_PARAHOTPLUG_STATE=%d",
> 		cmd->device_change_state.state.active);
> 	sprintf(env_bus, "VISOR_PARAHOTPLUG_BUS=%d",
> 		cmd->device_change_state.bus_no);
> 	sprintf(env_dev, "VISOR_PARAHOTPLUG_DEVICE=%d",
> 		cmd->device_change_state.dev_no >> 3);
> 	sprintf(env_func, "VISOR_PARAHOTPLUG_FUNCTION=%d",
> 		cmd->device_change_state.dev_no & 0x7);
> 	return kobject_uevent_env(&chipset_dev->acpi_device->dev.kobj,
> 				  KOBJ_CHANGE, envp);
> }
> 
> If this is not the sort of bug report you like, could you please (if you
> have time) school me on how to better report such things in the future.
> 
> thanks,
> Tobin.


I'll let Joe talk to whether this ought to be a CHECK.

The function there looks to never modify the envp pointer so some kind
of const might well be reasonable as you do not update the pointers.

Right?

-apw