Hi Shyam,
On 10/9/23 11:45, Shyam Sundar S K wrote:
> Currently in amd_pmc_stb_debugfs_open_v2() the buffer size is assumed to
> be fixed and a second call to amd_pmc_stb_debugfs_open_v2() may race with
> a process holding open another fd. This could change "fsize" to a
> bigger size causing an out of bounds read.
>
> Instead create a struct with a flexarray to solve this.
>
> Suggested-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@xxxxxxx>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> ---
> v5:
> - new patch based on comments in v4 from Hans.
> - based on review-ilpo branch
>
> drivers/platform/x86/amd/pmc/pmc.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index c92dd5077a16..e00d69801369 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -122,6 +122,11 @@ enum s2d_arg {
> S2D_DRAM_SIZE,
> };
>
> +struct amd_pmc_stb_v2_data {
> + size_t size;
> + u8 data[] __counted_by(size);
> +};
> +
> struct amd_pmc_bit_map {
> const char *name;
> u32 bit_mask;
> @@ -239,7 +244,8 @@ static const struct file_operations amd_pmc_stb_debugfs_fops = {
> static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
> {
> struct amd_pmc_dev *dev = filp->f_inode->i_private;
> - u32 *buf, fsize, num_samples, val, stb_rdptr_offset = 0;
> + u32 fsize, num_samples, val, stb_rdptr_offset = 0;
> + struct amd_pmc_stb_v2_data *flex_arr;
> int ret;
>
> /* Write dummy postcode while reading the STB buffer */
> @@ -247,10 +253,6 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
> if (ret)
> dev_err(dev->dev, "error writing to STB: %d\n", ret);
>
> - buf = kzalloc(S2D_TELEMETRY_BYTES_MAX, GFP_KERNEL);
> - if (!buf)
> - return -ENOMEM;
> -
> /* Spill to DRAM num_samples uses separate SMU message port */
> dev->msg_port = 1;
>
> @@ -264,7 +266,6 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
> dev->msg_port = 0;
> if (ret) {
> dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
> - kfree(buf);
> return ret;
> }
>
> @@ -277,8 +278,13 @@ static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
> stb_rdptr_offset = 0;
> }
>
> - memcpy_fromio(buf, dev->stb_virt_addr + stb_rdptr_offset, fsize);
> - filp->private_data = buf;
> + flex_arr = kzalloc(struct_size(flex_arr, data, fsize), GFP_KERNEL);
> + if (!flex_arr)
> + return -ENOMEM;
> +
> + memcpy_fromio(flex_arr->data, dev->stb_virt_addr + stb_rdptr_offset, fsize);
> + flex_arr->size = fsize;
> + filp->private_data = flex_arr->data;
>
> return 0;
> }
You are not adjusting amd_pmc_stb_debugfs_read_v2() to match it will still
return S2D_TELEMETRY_BYTES_MAX, likely overflowing the array!
And it will now return the contents of the size_t prefixing the buffer to
userspace.
So this is obviously broken, nack.
Regards,
Hans
