Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <dac24fac-57eb-4ca8-b819-fbdc24464d94@acm.org>
Date:   Tue, 12 Dec 2023 12:14:14 -0800
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] scsi: myrb: Fix a potential string truncation in
 rebuild_show()
Content-Language: en-US
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        hare@kernel.org, jejb@linux.ibm.com, martin.petersen@oracle.com
Cc:     hare@suse.com, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
References: <cover.1702411083.git.christophe.jaillet@wanadoo.fr>
 <2d3096dd4b1b6e758287e4062e3147c57c007eaa.1702411083.git.christophe.jaillet@wanadoo.fr>
From:   Bart Van Assche <bvanassche@acm.org>
In-Reply-To: <2d3096dd4b1b6e758287e4062e3147c57c007eaa.1702411083.git.christophe.jaillet@wanadoo.fr>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 12/12/23 10:09, Christophe JAILLET wrote:
> "physical device - not rebuilding\n" is 34 bytes long. When written in
> 'buf' with a limit of 32 bytes, it is truncated.
> 
> When building with W=1, it leads to:
>     drivers/scsi/myrb.c: In function ‘rebuild_show’:
>     drivers/scsi/myrb.c:1906:24: error: ‘physical device - not rebuil...’ directive output truncated writing 33 bytes into a region of size 32 [-Werror=format-truncation=]
>      1906 |                 return snprintf(buf, 32, "physical device - not rebuilding\n");
>           |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     drivers/scsi/myrb.c:1906:24: note: ‘snprintf’ output 34 bytes into a destination of size 32
> 
> Change the allowed size to 64 to fix the issue.
> 
> Fixes: 081ff398c56c ("scsi: myrb: Add Mylex RAID controller (block interface)")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>   drivers/scsi/myrb.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/myrb.c b/drivers/scsi/myrb.c
> index ca2e932dd9b7..ca2380d2d6d3 100644
> --- a/drivers/scsi/myrb.c
> +++ b/drivers/scsi/myrb.c
> @@ -1903,15 +1903,15 @@ static ssize_t rebuild_show(struct device *dev,
>   	unsigned char status;
>   
>   	if (sdev->channel < myrb_logical_channel(sdev->host))
> -		return snprintf(buf, 32, "physical device - not rebuilding\n");
> +		return snprintf(buf, 64, "physical device - not rebuilding\n");
>   
>   	status = myrb_get_rbld_progress(cb, &rbld_buf);
>   
>   	if (rbld_buf.ldev_num != sdev->id ||
>   	    status != MYRB_STATUS_SUCCESS)
> -		return snprintf(buf, 32, "not rebuilding\n");
> +		return snprintf(buf, 64, "not rebuilding\n");
>   
> -	return snprintf(buf, 32, "rebuilding block %u of %u\n",
> +	return snprintf(buf, 64, "rebuilding block %u of %u\n",
>   			rbld_buf.ldev_size - rbld_buf.blocks_left,
>   			rbld_buf.ldev_size);
>   }

Anyone who sees the resulting code without having seen the above patch will
wonder where the magic number '64' comes from. Please use sysfs_emit() instead
of snprintf(buf, 64, ...).

Thanks,

Bart.