Hi Shyam,
On 10/9/23 12:09, Shyam Sundar S K wrote:
> Hi Hans,
>
> On 10/9/2023 3:21 PM, 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;
>>>
>>> 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.
>
> I think it was taken care in v5 4/4. Can you please take a look at it
> and see if something is really missing?
Please see my other emails in the thread, this really is broken, by
still using a size variable in the global data struct you are
not fixing the race of 2 amd_pmc_stb_debugfs_open_v2() being
made shortly behind each other and
by storing:
filp->private_data = flex_arr->data;
instead of:
filp->private_data = flex_arr;
You are causing a pointer not returned by kmalloc to get
passed to kfree() in release().
Regards,
Hans
>
> return simple_read_from_buffer(buf, size, pos, filp->private_data,
> - S2D_TELEMETRY_BYTES_MAX);
> + dev->fsize);
>
>
> Thanks,
> Shyam
>
>>
>> Regards,
>>
>> Hans
>>
>>
>
