re: btrfs: add btrfs_read_policy_to_enum helper and refactor read, policy store

re: btrfs: add btrfs_read_policy_to_enum helper and refactor read, policy store

[Colin King (gmail)]

Hi,

Static analysis on linux-next today has found a potential buffer 
overflow in fs/btrfs/sysfs.c in function btrfs_read_policy_to_enum()

The strcpy to string param has no length checks on str and hence if str 
is longer than param a buffer overflow on the stack occurs. This can 
potentially occur when a user writes a long string to the btrfs sysfs 
file read_policy via btrfs_read_policy_store()

int btrfs_read_policy_to_enum(const char *str, s64 *value)
{
         char param[32] = {'\0'};
         char *__maybe_unused value_str;
         int index;
         bool found = false;

         if (!str || strlen(str) == 0)
                 return 0;

         strcpy(param, str);   /* issue here */

	....
}

Colin

Re: btrfs: add btrfs_read_policy_to_enum helper and refactor read, policy store

[Anand Jain]

Hi,

  The changes have already been submitted and will be included in the 
next re-roll.

  https://patchwork.kernel.org/project/linux-btrfs/patch/9fcc9f01bdab846db231b427f98fbb3e9df7c7a5.1734370092.git.anand.jain@oracle.com/#26168805


Thanks,
Anand

On 19/12/24 21:10, Colin King (gmail) wrote:
> Hi,
> 
> Static analysis on linux-next today has found a potential buffer 
> overflow in fs/btrfs/sysfs.c in function btrfs_read_policy_to_enum()
> 
> The strcpy to string param has no length checks on str and hence if str 
> is longer than param a buffer overflow on the stack occurs. This can 
> potentially occur when a user writes a long string to the btrfs sysfs 
> file read_policy via btrfs_read_policy_store()
> 
> int btrfs_read_policy_to_enum(const char *str, s64 *value)
> {
>          char param[32] = {'\0'};
>          char *__maybe_unused value_str;
>          int index;
>          bool found = false;
> 
>          if (!str || strlen(str) == 0)
>                  return 0;
> 
>          strcpy(param, str);   /* issue here */
> 
>      ....
> }
> 
> Colin

Re: btrfs: add btrfs_read_policy_to_enum helper and refactor read, policy store

[Colin King (gmail)]

On 19/12/2024 18:59, Anand Jain wrote:
> 
> Hi,
> 
>   The changes have already been submitted and will be included in the 
> next re-roll.

Thanks for the update.

I noticed the fix was:

strncpy(param, str, 31);

perhaps that should be based on the sizeof param, e.g. sizeof(param) - 1 
rather than a hard-coded 31 just in case the size of param is changed in 
the future.

Colin

> 
>   https://patchwork.kernel.org/project/linux-btrfs/ 
> patch/9fcc9f01bdab846db231b427f98fbb3e9df7c7a5.1734370092.git.anand.jain@oracle.com/#26168805
> 
> 
> Thanks,
> Anand
> 
> On 19/12/24 21:10, Colin King (gmail) wrote:
>> Hi,
>>
>> Static analysis on linux-next today has found a potential buffer 
>> overflow in fs/btrfs/sysfs.c in function btrfs_read_policy_to_enum()
>>
>> The strcpy to string param has no length checks on str and hence if 
>> str is longer than param a buffer overflow on the stack occurs. This 
>> can potentially occur when a user writes a long string to the btrfs 
>> sysfs file read_policy via btrfs_read_policy_store()
>>
>> int btrfs_read_policy_to_enum(const char *str, s64 *value)
>> {
>>          char param[32] = {'\0'};
>>          char *__maybe_unused value_str;
>>          int index;
>>          bool found = false;
>>
>>          if (!str || strlen(str) == 0)
>>                  return 0;
>>
>>          strcpy(param, str);   /* issue here */
>>
>>      ....
>> }
>>
>> Colin
> 

Re: btrfs: add btrfs_read_policy_to_enum helper and refactor read, policy store

[Anand Jain]

On 20/12/24 00:38, Colin King (gmail) wrote:
> On 19/12/2024 18:59, Anand Jain wrote:
>>
>> Hi,
>>
>>   The changes have already been submitted and will be included in the 
>> next re-roll.
> 
> Thanks for the update.
> 
> I noticed the fix was:
> 
> strncpy(param, str, 31);
> 
> perhaps that should be based on the sizeof param, e.g. sizeof(param) - 1 
> rather than a hard-coded 31 just in case the size of param is changed in 
> the future.
> 

  Good point, I should have used sizeof(param) - 1.

Thx, Anand

> Colin
> 
>>
>>   https://patchwork.kernel.org/project/linux-btrfs/ 
>> patch/9fcc9f01bdab846db231b427f98fbb3e9df7c7a5.1734370092.git.anand.jain@oracle.com/#26168805
>>
>>
>> Thanks,
>> Anand
>>
>> On 19/12/24 21:10, Colin King (gmail) wrote:
>>> Hi,
>>>
>>> Static analysis on linux-next today has found a potential buffer 
>>> overflow in fs/btrfs/sysfs.c in function btrfs_read_policy_to_enum()
>>>
>>> The strcpy to string param has no length checks on str and hence if 
>>> str is longer than param a buffer overflow on the stack occurs. This 
>>> can potentially occur when a user writes a long string to the btrfs 
>>> sysfs file read_policy via btrfs_read_policy_store()
>>>
>>> int btrfs_read_policy_to_enum(const char *str, s64 *value)
>>> {
>>>          char param[32] = {'\0'};
>>>          char *__maybe_unused value_str;
>>>          int index;
>>>          bool found = false;
>>>
>>>          if (!str || strlen(str) == 0)
>>>                  return 0;
>>>
>>>          strcpy(param, str);   /* issue here */
>>>
>>>      ....
>>> }
>>>
>>> Colin
>>
>