[1/2] proc: fix to check name length in proc_lookup_de()

Message ID 20230131155559.35800-1-chao@kernel.org
State New
Headers
Series [1/2] proc: fix to check name length in proc_lookup_de() |

Commit Message

Chao Yu Jan. 31, 2023, 3:55 p.m. UTC
  __proc_create() has limited dirent's max name length with 255, let's
add this limitation in proc_lookup_de(), so that it can return
-ENAMETOOLONG correctly instead of -ENOENT when stating a file which
has out-of-range name length.

Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/proc/generic.c  | 5 ++++-
 fs/proc/internal.h | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)
  

Comments

Chao Yu Feb. 1, 2023, 1:01 p.m. UTC | #1
Hi Andrew,

Could you please take a look at this patchset? Or should I ping
Alexey Dobriyan?

Thanks,

On 2023/1/31 23:55, Chao Yu wrote:
> __proc_create() has limited dirent's max name length with 255, let's
> add this limitation in proc_lookup_de(), so that it can return
> -ENAMETOOLONG correctly instead of -ENOENT when stating a file which
> has out-of-range name length.
> 
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>   fs/proc/generic.c  | 5 ++++-
>   fs/proc/internal.h | 3 +++
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> index 878d7c6db919..f547e9593a77 100644
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -245,6 +245,9 @@ struct dentry *proc_lookup_de(struct inode *dir, struct dentry *dentry,
>   {
>   	struct inode *inode;
>   
> +	if (dentry->d_name.len > PROC_NAME_LEN)
> +		return ERR_PTR(-ENAMETOOLONG);
> +
>   	read_lock(&proc_subdir_lock);
>   	de = pde_subdir_find(de, dentry->d_name.name, dentry->d_name.len);
>   	if (de) {
> @@ -401,7 +404,7 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
>   		goto out;
>   	qstr.name = fn;
>   	qstr.len = strlen(fn);
> -	if (qstr.len == 0 || qstr.len >= 256) {
> +	if (qstr.len == 0 || qstr.len > PROC_NAME_LEN) {
>   		WARN(1, "name len %u\n", qstr.len);
>   		return NULL;
>   	}
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index b701d0207edf..7611bc684d9e 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -142,6 +142,9 @@ unsigned name_to_int(const struct qstr *qstr);
>   /* Worst case buffer size needed for holding an integer. */
>   #define PROC_NUMBUF 13
>   
> +/* Max name length of procfs dirent */
> +#define PROC_NAME_LEN		255
> +
>   /*
>    * array.c
>    */
  
Andrew Morton Feb. 2, 2023, 11:41 p.m. UTC | #2
On Wed, 1 Feb 2023 21:01:14 +0800 Chao Yu <chao@kernel.org> wrote:

> Hi Andrew,
> 
> Could you please take a look at this patchset? Or should I ping
> Alexey Dobriyan?
> 

[patch 1/2]: Alexey wasn't keen on the v1 patch.  What changed?

[patch 2/2]: What is the benefit from this change?
  
Alexey Dobriyan Feb. 5, 2023, 12:21 p.m. UTC | #3
On Thu, Feb 02, 2023 at 03:41:54PM -0800, Andrew Morton wrote:
> On Wed, 1 Feb 2023 21:01:14 +0800 Chao Yu <chao@kernel.org> wrote:
> 
> > Hi Andrew,
> > 
> > Could you please take a look at this patchset? Or should I ping
> > Alexey Dobriyan?
> > 
> 
> [patch 1/2]: Alexey wasn't keen on the v1 patch.  What changed?

Nothing! /proc lived without this check for 30 years:

int proc_match(int len,const char * name,struct proc_dir_entry * de)
{
        register int same __asm__("ax");

        if (!de || !de->low_ino)
                return 0;
        /* "" means "." ---> so paths like "/usr/lib//libc.a" work */
        if (!len && (de->name[0]=='.') && (de->name[1]=='\0'))
                return 1;
        if (de->namelen != len)
                return 0;
        __asm__("cld\n\t"
                "repe ; cmpsb\n\t"
                "setz %%al"
                :"=a" (same)
                :"0" (0),"S" ((long) name),"D" ((long) de->name),"c" (len)
                :"cx","di","si");
        return same;
}
  
Chao Yu Feb. 11, 2023, 1:13 a.m. UTC | #4
On 2023/2/3 7:41, Andrew Morton wrote:
> [patch 1/2]: Alexey wasn't keen on the v1 patch.  What changed?

I replied to Alexey's comments on v1, but still didn't get any
feedback, so I just rebase the code to last -next. Sorry, too rush
to missed to add change log on v2.

> 
> [patch 2/2]: What is the benefit from this change?

Block size is mismatch in between results of stat() and statfs(),
this patch tries to fix this issue.

stat  /proc/
    File: /proc/
    Size: 0         	Blocks: 0          IO Block: 1024   directory
Device: 14h/20d	Inode: 1           Links: 310
Access: (0555/dr-xr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2023-01-28 23:14:20.491937242 +0800
Modify: 2023-01-28 23:14:20.491937242 +0800
Change: 2023-01-28 23:14:20.491937242 +0800
   Birth: -

stat -f  /proc/
    File: "/proc/"
      ID: 0        Namelen: 255     Type: proc
Block size: 4096       Fundamental block size: 4096
Blocks: Total: 0          Free: 0          Available: 0
Inodes: Total: 0          Free: 0
  
Chao Yu Feb. 11, 2023, 1:22 a.m. UTC | #5
On 2023/2/5 20:21, Alexey Dobriyan wrote:
> Nothing! /proc lived without this check for 30 years:

Oh, I'm trying make error handling logic of proc's lookup() to be the
same as other generic filesystem, it looks you don't think it's worth or
necessary to do that, anyway, the change is not critial, let's ignore it,
thank you for reviewing this patch.

Thanks,

> 
> int proc_match(int len,const char * name,struct proc_dir_entry * de)
> {
>          register int same __asm__("ax");
> 
>          if (!de || !de->low_ino)
>                  return 0;
>          /* "" means "." ---> so paths like "/usr/lib//libc.a" work */
>          if (!len && (de->name[0]=='.') && (de->name[1]=='\0'))
>                  return 1;
>          if (de->namelen != len)
>                  return 0;
>          __asm__("cld\n\t"
>                  "repe ; cmpsb\n\t"
>                  "setz %%al"
>                  :"=a" (same)
>                  :"0" (0),"S" ((long) name),"D" ((long) de->name),"c" (len)
>                  :"cx","di","si");
>          return same;
> }
  

Patch

diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index 878d7c6db919..f547e9593a77 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -245,6 +245,9 @@  struct dentry *proc_lookup_de(struct inode *dir, struct dentry *dentry,
 {
 	struct inode *inode;
 
+	if (dentry->d_name.len > PROC_NAME_LEN)
+		return ERR_PTR(-ENAMETOOLONG);
+
 	read_lock(&proc_subdir_lock);
 	de = pde_subdir_find(de, dentry->d_name.name, dentry->d_name.len);
 	if (de) {
@@ -401,7 +404,7 @@  static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
 		goto out;
 	qstr.name = fn;
 	qstr.len = strlen(fn);
-	if (qstr.len == 0 || qstr.len >= 256) {
+	if (qstr.len == 0 || qstr.len > PROC_NAME_LEN) {
 		WARN(1, "name len %u\n", qstr.len);
 		return NULL;
 	}
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index b701d0207edf..7611bc684d9e 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -142,6 +142,9 @@  unsigned name_to_int(const struct qstr *qstr);
 /* Worst case buffer size needed for holding an integer. */
 #define PROC_NUMBUF 13
 
+/* Max name length of procfs dirent */
+#define PROC_NAME_LEN		255
+
 /*
  * array.c
  */