Hi Shyam,
On 10/9/23 11:51, Hans de Goede wrote:
> 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;
So I just notices this, this means that amd_pmc_stb_debugfs_read_v2()
will still start with sending the data and won't prefix it with the
size as I initially thought. I really expected this to read:
filp->private_data = flex_arr;
Note that although this fixes the prefixing with size issue,
it still leaves the amd_pmc_stb_debugfs_read_v2() returning
S2D_TELEMETRY_BYTES_MAX bytes which overruns the buffer issue.
And it makes amd_pmc_stb_debugfs_release_v2() now call kfree()
on a pointer which was not returned by kmalloc, since
you are now calling kfree(&(flex_arr->data[0]), which
is sizeof(size_t) bytes after the pointer returned by
the kzalloc.
So things are still broken here, you really should set:
filp->private_data = flex_arr;
So that amd_pmc_stb_debugfs_release_v2() frees the correct
pointer and then change amd_pmc_stb_debugfs_read_v2() to:
static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char __user *buf, size_t size,
loff_t *pos)
{
struct amd_pmc_stb_v2_data *data = filp->private_data;
return simple_read_from_buffer(buf, size, pos, data->data, data->size);
}
Regards,
Hans
>>
>> 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
>
>
