linux-kernel - Re: [PATCH 5/7] scsi: ufs: Refactor descriptor read for write

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, 4 Jun 2018 08:40:44 +0000
From:   Bart Van Assche <Bart.VanAssche@....com>
To:     "jejb@...ux.vnet.ibm.com" <jejb@...ux.vnet.ibm.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "evgreen@...omium.org" <evgreen@...omium.org>,
        "vinholikatti@...il.com" <vinholikatti@...il.com>,
        Stanislav Nijnikov <Stanislav.Nijnikov@....com>
CC:     "gwendal@...omium.org" <gwendal@...omium.org>
Subject: Re: [PATCH 5/7] scsi: ufs: Refactor descriptor read for write

On Tue, 2018-05-29 at 11:17 -0700, Evan Green wrote:
>  	/* Check whether we need temp memory */
>  	if (param_offset != 0 || param_size < buff_len) {
> -		desc_buf = kmalloc(buff_len, GFP_KERNEL);
> +		desc_buf = kzalloc(buff_len, GFP_KERNEL);
>  		if (!desc_buf)
>  			return -ENOMEM;
> +
> +		/* If it's a write, first read the complete descriptor, then
> +		 * copy in the parts being changed.
> +		 */

Have you verified this patch with checkpatch? The above comment does not follow
the Linux kernel coding style.

> +		if (opcode == UPIU_QUERY_OPCODE_WRITE_DESC) {
> +			if ((int)param_offset + (int)param_size > buff_len) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +
> +			ret = ufshcd_query_descriptor_retry(hba,
> +						UPIU_QUERY_OPCODE_READ_DESC,
> +						desc_id, desc_index, 0,
> +						desc_buf, &buff_len);
> +
> +			if (ret) {
> +				dev_err(hba->dev,
> +					"%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d",
> +					__func__, desc_id, desc_index,
> +					param_offset, ret);
> +
> +				goto out;
> +			}
> +
> +			memcpy(desc_buf + param_offset, param_buf, param_size);
> +		}

The above code is indented deeply. I think that means that this code would become
easier to read if a helper function would be introduced.

Additionally, I think locking is missing from the above code. How else can race
conditions between concurrent writers be prevented?

Bart.