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

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

[Chao Yu]

__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
  */
-- 
2.36.1

[PATCH 2/2] proc: fix .s_blocksize and .s_blocksize_bits

[Chao Yu]

procfs uses seq_file's operations to process IO, and seq_file
uses PAGE_SIZE as basic operating unit, so, fix to update
.s_blocksize and .s_blocksize_bits from 1024 and 10 to PAGE_SIZE
and PAGE_SHIFT.

Signed-off-by: Chao Yu <chao@kernel.org>
---
v2:
- fix to update blocksize to PAGE_SIZE pointed out by Alexey Dobriyan.
 fs/proc/root.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/proc/root.c b/fs/proc/root.c
index 3c2ee3eb1138..8bf5a9080adc 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -173,8 +173,8 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
 	/* User space would break if executables or devices appear on proc */
 	s->s_iflags |= SB_I_USERNS_VISIBLE | SB_I_NOEXEC | SB_I_NODEV;
 	s->s_flags |= SB_NODIRATIME | SB_NOSUID | SB_NOEXEC;
-	s->s_blocksize = 1024;
-	s->s_blocksize_bits = 10;
+	s->s_blocksize = PAGE_SIZE;
+	s->s_blocksize_bits = PAGE_SHIFT;
 	s->s_magic = PROC_SUPER_MAGIC;
 	s->s_op = &proc_sops;
 	s->s_time_gran = 1;
-- 
2.36.1

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

[Chao Yu]

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
>    */

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

[Andrew Morton]

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?

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

[Alexey Dobriyan]

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;
}

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

[Chao Yu]

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

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

[Chao Yu]

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;
> }