linux-kernel - Re: [PATCH v4 2/2] RDMA/srp: Fix error return code in srp_parse

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]

Message-ID: <b4d78eb0-4492-ac7f-d500-3d49b0dc7aa1@acm.org>
Date:   Tue, 29 Nov 2022 10:43:53 -0800
From:   Bart Van Assche <bvanassche@....org>
To:     Wang Yufen <wangyufen@...wei.com>, jgg@...pe.ca, leon@...nel.org,
        dennis.dalessandro@...nelisnetworks.com
Cc:     linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org,
        andriy.shevchenko@...ux.intel.com, bart.vanassche@....com,
        easwar.hariharan@...el.com
Subject: Re: [PATCH v4 2/2] RDMA/srp: Fix error return code in
 srp_parse_options()

On 11/28/22 18:04, Wang Yufen wrote:
> In the previous while loop, "ret" may be assigned zero, , so the error

The word "iteration" is missing from the above sentence. Additionally, 
there is a double comma.

> return code may be incorrectly set to 0 instead of -EINVAL. Alse

Alse -> Also

>   		case SRP_OPT_QUEUE_SIZE:
> -			if (match_int(args, &token) || token < 1) {
> +			ret = match_int(args, &token);
> +			if (ret)
> +				goto out;
> +			if (token < 1) {
>   				pr_warn("bad queue_size parameter '%s'\n", p);
> +				ret = -EINVAL;
>   				goto out;
>   			}
>   			target->scsi_host->can_queue = token;

Why only to report "bad queue_size parameter" if ret == 0 && token < 1 
but not if ret < 0? This is a behavior change that has not been 
explained in the patch description.

> @@ -3490,25 +3496,34 @@ static int srp_parse_options(struct net *net, const char *buf,
>   			break;
>   
>   		case SRP_OPT_MAX_CMD_PER_LUN:
> -			if (match_int(args, &token) || token < 1) {
> +			ret = match_int(args, &token);
> +			if (ret)
> +				goto out;
> +			if (token < 1) {
>   				pr_warn("bad max cmd_per_lun parameter '%s'\n",
>   					p);
> +				ret = -EINVAL;
>   				goto out;
>   			}
>   			target->scsi_host->cmd_per_lun = token;
>   			break;

Same comment here and for many other changes below.

Thanks,

Bart.