| Message ID | 20241030130308.1066299-5-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: add more kernel parameters to control mTHP | expand |
Hi Maíra, Looks like a lot of kernel code is still using strcpy() ;p On Wed, Oct 30, 2024 at 9:03 PM Maíra Canal <mcanal@igalia.com> wrote: > > Replace strcpy() with strscpy() in mm/huge_memory.c > > strcpy() has been deprecated because it is generally unsafe, so help to > eliminate it from the kernel source. > > Link: https://github.com/KSPP/linux/issues/88 > Signed-off-by: Maíra Canal <mcanal@igalia.com> Feel free to add: Reviewed-by: Lance Yang <ioworker0@gmail.com> Thanks, Lance > --- > mm/huge_memory.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index f92068864469..8f41a694433c 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str) > > if (!str || strlen(str) + 1 > PAGE_SIZE) > goto err; > - strcpy(str_dup, str); > + strscpy(str_dup, str); > > always = huge_anon_orders_always; > madvise = huge_anon_orders_madvise; > @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf, > > tok = strsep(&buf, ","); > if (tok) { > - strcpy(file_path, tok); > + strscpy(file_path, tok); > } else { > ret = -EINVAL; > goto out; > -- > 2.46.2 >
Hi Barry, On 30/10/24 20:07, Barry Song wrote: > On Thu, Oct 31, 2024 at 2:03 AM Maíra Canal <mcanal@igalia.com> wrote: >> >> Replace strcpy() with strscpy() in mm/huge_memory.c >> >> strcpy() has been deprecated because it is generally unsafe, so help to >> eliminate it from the kernel source. >> >> Link: https://github.com/KSPP/linux/issues/88 >> Signed-off-by: Maíra Canal <mcanal@igalia.com> >> --- >> mm/huge_memory.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index f92068864469..8f41a694433c 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str) >> >> if (!str || strlen(str) + 1 > PAGE_SIZE) >> goto err; >> - strcpy(str_dup, str); >> + strscpy(str_dup, str); > > What is the difference between strcpy and strscpy without a size parameter? > > we have already a check and goto err. strcpy() is entirely safe. > if (!str || strlen(str) + 1 > PAGE_SIZE) > goto err; > > My understanding is that we don't need this patch. strcpy() is a deprecated interface [1]. From the GitHub issue I linked in the commit description, Kees states: "A lot of kernel code is still using strcpy(). While the CONFIG_FORTIFY_SOURCE wrapper macros tend to make its use mostly safe, it would be nice to eliminate the function from the kernel entirely." [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy Best Regards, - Maíra > >> >> always = huge_anon_orders_always; >> madvise = huge_anon_orders_madvise; >> @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf, >> >> tok = strsep(&buf, ","); >> if (tok) { >> - strcpy(file_path, tok); >> + strscpy(file_path, tok); >> } else { >> ret = -EINVAL; >> goto out; >> -- >> 2.46.2 >> > > Thanks > barry
On Thu, Oct 31, 2024 at 6:55 PM Maíra Canal <mcanal@igalia.com> wrote: > > Hi Barry, > > On 30/10/24 20:07, Barry Song wrote: > > On Thu, Oct 31, 2024 at 2:03 AM Maíra Canal <mcanal@igalia.com> wrote: > >> > >> Replace strcpy() with strscpy() in mm/huge_memory.c > >> > >> strcpy() has been deprecated because it is generally unsafe, so help to > >> eliminate it from the kernel source. > >> > >> Link: https://github.com/KSPP/linux/issues/88 > >> Signed-off-by: Maíra Canal <mcanal@igalia.com> > >> --- > >> mm/huge_memory.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > >> index f92068864469..8f41a694433c 100644 > >> --- a/mm/huge_memory.c > >> +++ b/mm/huge_memory.c > >> @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str) > >> > >> if (!str || strlen(str) + 1 > PAGE_SIZE) > >> goto err; > >> - strcpy(str_dup, str); > >> + strscpy(str_dup, str); > > > > What is the difference between strcpy and strscpy without a size parameter? > > > > we have already a check and goto err. strcpy() is entirely safe. > > if (!str || strlen(str) + 1 > PAGE_SIZE) > > goto err; > > > > My understanding is that we don't need this patch. > > strcpy() is a deprecated interface [1]. From the GitHub issue I linked > in the commit description, Kees states: "A lot of kernel code is still > using strcpy(). While the CONFIG_FORTIFY_SOURCE wrapper macros tend to > make its use mostly safe, it would be nice to eliminate the function > from the kernel entirely." I don't see any value added here since strscpy() has no size parameter and we have checked strlen(str) + 1 > PAGE_SIZE to avoid parsing any pointless bootcmd longer than PAGE_SIZE. But I have no objection to this patch if other people like it. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy > > Best Regards, > - Maíra > > > > >> > >> always = huge_anon_orders_always; > >> madvise = huge_anon_orders_madvise; > >> @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf, > >> > >> tok = strsep(&buf, ","); > >> if (tok) { > >> - strcpy(file_path, tok); > >> + strscpy(file_path, tok); > >> } else { > >> ret = -EINVAL; > >> goto out; > >> -- > >> 2.46.2 > >> > > Thanks barry
+cc Kees Cook Hi Barry, On 31/10/24 09:01, Barry Song wrote: > On Thu, Oct 31, 2024 at 6:55 PM Maíra Canal <mcanal@igalia.com> wrote: >> >> Hi Barry, >> >> On 30/10/24 20:07, Barry Song wrote: >>> On Thu, Oct 31, 2024 at 2:03 AM Maíra Canal <mcanal@igalia.com> wrote: >>>> >>>> Replace strcpy() with strscpy() in mm/huge_memory.c >>>> >>>> strcpy() has been deprecated because it is generally unsafe, so help to >>>> eliminate it from the kernel source. >>>> >>>> Link: https://github.com/KSPP/linux/issues/88 >>>> Signed-off-by: Maíra Canal <mcanal@igalia.com> >>>> --- >>>> mm/huge_memory.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>>> index f92068864469..8f41a694433c 100644 >>>> --- a/mm/huge_memory.c >>>> +++ b/mm/huge_memory.c >>>> @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str) >>>> >>>> if (!str || strlen(str) + 1 > PAGE_SIZE) >>>> goto err; >>>> - strcpy(str_dup, str); >>>> + strscpy(str_dup, str); >>> >>> What is the difference between strcpy and strscpy without a size parameter? >>> >>> we have already a check and goto err. strcpy() is entirely safe. >>> if (!str || strlen(str) + 1 > PAGE_SIZE) >>> goto err; >>> >>> My understanding is that we don't need this patch. >> >> strcpy() is a deprecated interface [1]. From the GitHub issue I linked >> in the commit description, Kees states: "A lot of kernel code is still >> using strcpy(). While the CONFIG_FORTIFY_SOURCE wrapper macros tend to >> make its use mostly safe, it would be nice to eliminate the function >> from the kernel entirely." > > I don't see any value added here since strscpy() has no size parameter and As `str_dup` is a sized buffer, we don't need to specify the size parameter. > we have checked strlen(str) + 1 > PAGE_SIZE to avoid parsing any pointless > bootcmd longer than PAGE_SIZE. From my point of view, this is an extra layer of safety. A developer could change `str_dup` to SZ_4K and leave PAGE_SIZE in the check. This could pass by in a review and we would have a check that allows strings bigger than the destination buffer in systems using 16KB pages. But see that `split_huge_pages_write` uses `strcpy` without any checks. I can remove the internal check from `setup_thp_anon` if you feel it would be more suitable. Best Regards, - Maíra > > But I have no objection to this patch if other people like it. > >> >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy >> >> Best Regards, >> - Maíra >> >>> >>>> >>>> always = huge_anon_orders_always; >>>> madvise = huge_anon_orders_madvise; >>>> @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf, >>>> >>>> tok = strsep(&buf, ","); >>>> if (tok) { >>>> - strcpy(file_path, tok); >>>> + strscpy(file_path, tok); >>>> } else { >>>> ret = -EINVAL; >>>> goto out; >>>> -- >>>> 2.46.2 >>>> >>> > Thanks > barry
On Fri, Nov 1, 2024 at 1:12 AM Maíra Canal <mcanal@igalia.com> wrote: > > +cc Kees Cook > > Hi Barry, > > On 31/10/24 09:01, Barry Song wrote: > > On Thu, Oct 31, 2024 at 6:55 PM Maíra Canal <mcanal@igalia.com> wrote: > >> > >> Hi Barry, > >> > >> On 30/10/24 20:07, Barry Song wrote: > >>> On Thu, Oct 31, 2024 at 2:03 AM Maíra Canal <mcanal@igalia.com> wrote: > >>>> > >>>> Replace strcpy() with strscpy() in mm/huge_memory.c > >>>> > >>>> strcpy() has been deprecated because it is generally unsafe, so help to > >>>> eliminate it from the kernel source. > >>>> > >>>> Link: https://github.com/KSPP/linux/issues/88 > >>>> Signed-off-by: Maíra Canal <mcanal@igalia.com> > >>>> --- > >>>> mm/huge_memory.c | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > >>>> index f92068864469..8f41a694433c 100644 > >>>> --- a/mm/huge_memory.c > >>>> +++ b/mm/huge_memory.c > >>>> @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str) > >>>> > >>>> if (!str || strlen(str) + 1 > PAGE_SIZE) > >>>> goto err; > >>>> - strcpy(str_dup, str); > >>>> + strscpy(str_dup, str); > >>> > >>> What is the difference between strcpy and strscpy without a size parameter? > >>> > >>> we have already a check and goto err. strcpy() is entirely safe. > >>> if (!str || strlen(str) + 1 > PAGE_SIZE) > >>> goto err; > >>> > >>> My understanding is that we don't need this patch. > >> > >> strcpy() is a deprecated interface [1]. From the GitHub issue I linked > >> in the commit description, Kees states: "A lot of kernel code is still > >> using strcpy(). While the CONFIG_FORTIFY_SOURCE wrapper macros tend to > >> make its use mostly safe, it would be nice to eliminate the function > >> from the kernel entirely." > > > > I don't see any value added here since strscpy() has no size parameter and > > As `str_dup` is a sized buffer, we don't need to specify the size > parameter. > > > we have checked strlen(str) + 1 > PAGE_SIZE to avoid parsing any pointless > > bootcmd longer than PAGE_SIZE. > > From my point of view, this is an extra layer of safety. A developer > could change `str_dup` to SZ_4K and leave PAGE_SIZE in the check. This > could pass by in a review and we would have a check that allows strings > bigger than the destination buffer in systems using 16KB pages. > > But see that `split_huge_pages_write` uses `strcpy` without any checks. > > I can remove the internal check from `setup_thp_anon` if you feel it > would be more suitable. My point is that we shouldn't remove the strlen(str) + 1 > PAGE_SIZE check, as it could lead to situations like this: For thp_anon=AB, where len(A) = PAGE_SIZE and B is illegal, we would mistakenly interpret the string as valid by ignoring B after trimming it. Therefore, if you remove the check, you could instead do: len = strscpy(str_dup, src); if (len >= sizeof(str_dup)) err; I have no objection to replacing strcpy with strscpy, so feel free to go ahead :-) > > Best Regards, > - Maíra > > > > But I have no objection to this patch if other people like it. > > > >> > >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy > >> > >> Best Regards, > >> - Maíra > >> > >>> > >>>> > >>>> always = huge_anon_orders_always; > >>>> madvise = huge_anon_orders_madvise; > >>>> @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf, > >>>> > >>>> tok = strsep(&buf, ","); > >>>> if (tok) { > >>>> - strcpy(file_path, tok); > >>>> + strscpy(file_path, tok); > >>>> } else { > >>>> ret = -EINVAL; > >>>> goto out; > >>>> -- > >>>> 2.46.2 > >>>> > >>> Thanks barry
On Fri, Nov 1, 2024 at 9:27 AM Barry Song <21cnbao@gmail.com> wrote: > > On Fri, Nov 1, 2024 at 1:12 AM Maíra Canal <mcanal@igalia.com> wrote: > > > > +cc Kees Cook > > > > Hi Barry, > > > > On 31/10/24 09:01, Barry Song wrote: > > > On Thu, Oct 31, 2024 at 6:55 PM Maíra Canal <mcanal@igalia.com> wrote: > > >> > > >> Hi Barry, > > >> > > >> On 30/10/24 20:07, Barry Song wrote: > > >>> On Thu, Oct 31, 2024 at 2:03 AM Maíra Canal <mcanal@igalia.com> wrote: > > >>>> > > >>>> Replace strcpy() with strscpy() in mm/huge_memory.c > > >>>> > > >>>> strcpy() has been deprecated because it is generally unsafe, so help to > > >>>> eliminate it from the kernel source. > > >>>> > > >>>> Link: https://github.com/KSPP/linux/issues/88 > > >>>> Signed-off-by: Maíra Canal <mcanal@igalia.com> > > >>>> --- > > >>>> mm/huge_memory.c | 4 ++-- > > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > > >>>> > > >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > >>>> index f92068864469..8f41a694433c 100644 > > >>>> --- a/mm/huge_memory.c > > >>>> +++ b/mm/huge_memory.c > > >>>> @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str) > > >>>> > > >>>> if (!str || strlen(str) + 1 > PAGE_SIZE) > > >>>> goto err; > > >>>> - strcpy(str_dup, str); > > >>>> + strscpy(str_dup, str); > > >>> > > >>> What is the difference between strcpy and strscpy without a size parameter? > > >>> > > >>> we have already a check and goto err. strcpy() is entirely safe. > > >>> if (!str || strlen(str) + 1 > PAGE_SIZE) > > >>> goto err; > > >>> > > >>> My understanding is that we don't need this patch. > > >> > > >> strcpy() is a deprecated interface [1]. From the GitHub issue I linked > > >> in the commit description, Kees states: "A lot of kernel code is still > > >> using strcpy(). While the CONFIG_FORTIFY_SOURCE wrapper macros tend to > > >> make its use mostly safe, it would be nice to eliminate the function > > >> from the kernel entirely." > > > > > > I don't see any value added here since strscpy() has no size parameter and > > > > As `str_dup` is a sized buffer, we don't need to specify the size > > parameter. > > > > > we have checked strlen(str) + 1 > PAGE_SIZE to avoid parsing any pointless > > > bootcmd longer than PAGE_SIZE. > > > > From my point of view, this is an extra layer of safety. A developer > > could change `str_dup` to SZ_4K and leave PAGE_SIZE in the check. This > > could pass by in a review and we would have a check that allows strings > > bigger than the destination buffer in systems using 16KB pages. > > > > But see that `split_huge_pages_write` uses `strcpy` without any checks. > > > > I can remove the internal check from `setup_thp_anon` if you feel it > > would be more suitable. > > My point is that we shouldn't remove the strlen(str) + 1 > PAGE_SIZE > check, as it could lead to situations like this: > > For thp_anon=AB, where len(A) = PAGE_SIZE and B is illegal, we would > mistakenly interpret the string as valid by ignoring B after trimming it. > > Therefore, if you remove the check, you could instead do: > > len = strscpy(str_dup, src); > if (len >= sizeof(str_dup)) > err; Perhaps -E2BIG ( or < 0) i'm not quite sure :-) > > I have no objection to replacing strcpy with strscpy, so feel free to go > ahead :-) > > > > > Best Regards, > > - Maíra > > > > > > But I have no objection to this patch if other people like it. > > > > > >> > > >> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy > > >> > > >> Best Regards, > > >> - Maíra > > >> > > >>> > > >>>> > > >>>> always = huge_anon_orders_always; > > >>>> madvise = huge_anon_orders_madvise; > > >>>> @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf, > > >>>> > > >>>> tok = strsep(&buf, ","); > > >>>> if (tok) { > > >>>> - strcpy(file_path, tok); > > >>>> + strscpy(file_path, tok); > > >>>> } else { > > >>>> ret = -EINVAL; > > >>>> goto out; > > >>>> -- > > >>>> 2.46.2 > > >>>> > > >>> > Thanks > barry
diff --git a/mm/huge_memory.c b/mm/huge_memory.c index f92068864469..8f41a694433c 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -989,7 +989,7 @@ static int __init setup_thp_anon(char *str) if (!str || strlen(str) + 1 > PAGE_SIZE) goto err; - strcpy(str_dup, str); + strscpy(str_dup, str); always = huge_anon_orders_always; madvise = huge_anon_orders_madvise; @@ -4175,7 +4175,7 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf, tok = strsep(&buf, ","); if (tok) { - strcpy(file_path, tok); + strscpy(file_path, tok); } else { ret = -EINVAL; goto out;
Replace strcpy() with strscpy() in mm/huge_memory.c strcpy() has been deprecated because it is generally unsafe, so help to eliminate it from the kernel source. Link: https://github.com/KSPP/linux/issues/88 Signed-off-by: Maíra Canal <mcanal@igalia.com> --- mm/huge_memory.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)