All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] avoid NULL deference in ext2_xattr_get
@ 2010-07-10  8:07 ` shenghui
  0 siblings, 0 replies; 10+ messages in thread
From: shenghui @ 2010-07-10  8:07 UTC (permalink / raw)
  To: kernel-janitors, linux-kernel, linux-ext4

Hi,

         I walked through ext2 code, and found one potential NULL deference
in ext2/xattr.c.  The version is 2.6.35-rc4, while earlier versions have the
same problem.
         If you configure EXT2_XATTR_DEBUG, you'll get:
# define ea_idebug(inode, f...) do { \
                printk(KERN_DEBUG "inode %s:%ld: ", \
                        inode->i_sb->s_id, inode->i_ino); \
                printk(f); \
                printk("\n"); \
        } while (0)

In ext2/xttr.c ext2_xattr_get, NULL pointer check is done after
ea_idebug call, so some may hit NULL deference here.
 ext2_xattr_get(struct inode *inode, int name_index, const char *name,
                void *buffer, size_t buffer_size)
 {
         struct buffer_head *bh = NULL;
         struct ext2_xattr_entry *entry;
         size_t name_len, size;
         char *end;
         int error;

         ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
                   name_index, name, buffer, (long)buffer_size);

         if (name == NULL)
                 return -EINVAL;


Following is my patch. Please check it.
The patch is against kernel 2.6.35-rc4.


>From adc1fa6535034db3b6d8deebda6ec7eaa8bfd2f8 Mon Sep 17 00:00:00 2001
From: Wang Sheng-Hui <crosslonelyover@gmail.com>
Date: Sat, 10 Jul 2010 16:05:53 +0800
Subject: [PATCH] avoid NULL deference in ext2_xattr_get


Signed-off-by: Wang Sheng-Hui <crosslonelyover@gmail.com>
---
 fs/ext2/xattr.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index 7c39157..81ec1c6 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -156,11 +156,12 @@ ext2_xattr_get(struct inode *inode, int
name_index, const char *name,
 	char *end;
 	int error;

+	if (name == NULL)
+		return -EINVAL;
+
 	ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
 		  name_index, name, buffer, (long)buffer_size);

-	if (name == NULL)
-		return -EINVAL;
 	down_read(&EXT2_I(inode)->xattr_sem);
 	error = -ENODATA;
 	if (!EXT2_I(inode)->i_file_acl)
-- 
1.6.3.3




-- 


Thanks and Best Regards,
shenghui

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH] avoid NULL deference in ext2_xattr_get
@ 2010-07-10  8:07 ` shenghui
  0 siblings, 0 replies; 10+ messages in thread
From: shenghui @ 2010-07-10  8:07 UTC (permalink / raw)
  To: kernel-janitors, linux-kernel, linux-ext4

Hi,

         I walked through ext2 code, and found one potential NULL deference
in ext2/xattr.c.  The version is 2.6.35-rc4, while earlier versions have the
same problem.
         If you configure EXT2_XATTR_DEBUG, you'll get:
# define ea_idebug(inode, f...) do { \
                printk(KERN_DEBUG "inode %s:%ld: ", \
                        inode->i_sb->s_id, inode->i_ino); \
                printk(f); \
                printk("\n"); \
        } while (0)

In ext2/xttr.c ext2_xattr_get, NULL pointer check is done after
ea_idebug call, so some may hit NULL deference here.
 ext2_xattr_get(struct inode *inode, int name_index, const char *name,
                void *buffer, size_t buffer_size)
 {
         struct buffer_head *bh = NULL;
         struct ext2_xattr_entry *entry;
         size_t name_len, size;
         char *end;
         int error;

         ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
                   name_index, name, buffer, (long)buffer_size);

         if (name = NULL)
                 return -EINVAL;


Following is my patch. Please check it.
The patch is against kernel 2.6.35-rc4.


From adc1fa6535034db3b6d8deebda6ec7eaa8bfd2f8 Mon Sep 17 00:00:00 2001
From: Wang Sheng-Hui <crosslonelyover@gmail.com>
Date: Sat, 10 Jul 2010 16:05:53 +0800
Subject: [PATCH] avoid NULL deference in ext2_xattr_get


Signed-off-by: Wang Sheng-Hui <crosslonelyover@gmail.com>
---
 fs/ext2/xattr.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index 7c39157..81ec1c6 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -156,11 +156,12 @@ ext2_xattr_get(struct inode *inode, int
name_index, const char *name,
 	char *end;
 	int error;

+	if (name = NULL)
+		return -EINVAL;
+
 	ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
 		  name_index, name, buffer, (long)buffer_size);

-	if (name = NULL)
-		return -EINVAL;
 	down_read(&EXT2_I(inode)->xattr_sem);
 	error = -ENODATA;
 	if (!EXT2_I(inode)->i_file_acl)
-- 
1.6.3.3




-- 


Thanks and Best Regards,
shenghui

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH] avoid NULL deference in ext2_xattr_get
  2010-07-10  8:07 ` shenghui
@ 2010-07-10  9:07   ` Dan Carpenter
  -1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2010-07-10  9:07 UTC (permalink / raw)
  To: shenghui; +Cc: kernel-janitors, linux-kernel, linux-ext4


Good catch.  Your change is correct but can you resend it?

On Sat, Jul 10, 2010 at 04:07:28PM +0800, shenghui wrote:
> >From adc1fa6535034db3b6d8deebda6ec7eaa8bfd2f8 Mon Sep 17 00:00:00 2001
> From: Wang Sheng-Hui <crosslonelyover@gmail.com>
> Date: Sat, 10 Jul 2010 16:05:53 +0800
> Subject: [PATCH] avoid NULL deference in ext2_xattr_get
> 

The longer description of the bug should go here.

> 
> Signed-off-by: Wang Sheng-Hui <crosslonelyover@gmail.com>
> ---
>  fs/ext2/xattr.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 7c39157..81ec1c6 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -156,11 +156,12 @@ ext2_xattr_get(struct inode *inode, int
> name_index, const char *name,
^^^^^^

Your email client line wrapped the patch.  Please send it to yourself
and then save the email including headers and everything.
	cd linux-2.6
	cat ~/your-email-text | patch -p1 

If it applies cleanly then resend it to the list.

regards,
dan carpenter



^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] avoid NULL deference in ext2_xattr_get
@ 2010-07-10  9:07   ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2010-07-10  9:07 UTC (permalink / raw)
  To: shenghui; +Cc: kernel-janitors, linux-kernel, linux-ext4


Good catch.  Your change is correct but can you resend it?

On Sat, Jul 10, 2010 at 04:07:28PM +0800, shenghui wrote:
> >From adc1fa6535034db3b6d8deebda6ec7eaa8bfd2f8 Mon Sep 17 00:00:00 2001
> From: Wang Sheng-Hui <crosslonelyover@gmail.com>
> Date: Sat, 10 Jul 2010 16:05:53 +0800
> Subject: [PATCH] avoid NULL deference in ext2_xattr_get
> 

The longer description of the bug should go here.

> 
> Signed-off-by: Wang Sheng-Hui <crosslonelyover@gmail.com>
> ---
>  fs/ext2/xattr.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 7c39157..81ec1c6 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -156,11 +156,12 @@ ext2_xattr_get(struct inode *inode, int
> name_index, const char *name,
^^^^^^

Your email client line wrapped the patch.  Please send it to yourself
and then save the email including headers and everything.
	cd linux-2.6
	cat ~/your-email-text | patch -p1 

If it applies cleanly then resend it to the list.

regards,
dan carpenter



^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] avoid NULL deference in ext2_xattr_get
  2010-07-10  8:07 ` shenghui
@ 2010-07-10  9:21   ` Dan Carpenter
  -1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2010-07-10  9:21 UTC (permalink / raw)
  To: shenghui; +Cc: kernel-janitors, linux-kernel, linux-ext4


Another way to fix this would be to say:

   	ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
-  		  name_index, name, buffer, (long)buffer_size);
+  		  name_index, name ?: "(null)", buffer, (long)buffer_size);

That would help for debugging.

regards,
dan carpenter



^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] avoid NULL deference in ext2_xattr_get
@ 2010-07-10  9:21   ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2010-07-10  9:21 UTC (permalink / raw)
  To: shenghui; +Cc: kernel-janitors, linux-kernel, linux-ext4


Another way to fix this would be to say:

   	ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
-  		  name_index, name, buffer, (long)buffer_size);
+  		  name_index, name ?: "(null)", buffer, (long)buffer_size);

That would help for debugging.

regards,
dan carpenter



^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] avoid NULL deference in ext2_xattr_get
  2010-07-10  8:07 ` shenghui
@ 2010-07-10 19:10   ` David Beal
  -1 siblings, 0 replies; 10+ messages in thread
From: David Beal @ 2010-07-10 19:10 UTC (permalink / raw)
  To: shenghui; +Cc: kernel-janitors, linux-kernel, linux-ext4


Hi Sheng-Hui,

There is no NULL dereference in this particular case.  In Linux's printing
routines, printk("%s",(char*)0) is safe, and would result in a "<NULL>" token
being printed.

Indicating that the "name" argument is "<NULL>" in the output of the ea_debug
routine is clearly valuable, as its purpose is to emit debugging messages.

If your patch were applied, then a log message indicating that "name" is
"<NULL>" would not appear.  Adding ternary operator inside printk() argument
body is also not necessary.

Specifically, in lib/vsnprintf.c, which services printk, a check against NULL
is made for the string format specifier, %s:

lib/vsnprintf.c:
...
static char *string(char *buf, char *end, char *s, struct printf_spec spec)
{
        int len, i;

        if ((unsigned long)s < PAGE_SIZE)
                s = "<NULL>";
...

The NULL check inside snprintf is GOOD, as it makes printk safer, and removes
the need for the thousands of conditional branches that would need to be done
OUTSIDE the printk function in order to check whether the argument to %s is
NULL.

Branch prediction has an easier time working with a branch in a function that
is more often in the cache.

Thank you,
David Beal


On Sat, Jul 10, 2010 at 04:07:28PM +0800, shenghui wrote:
> Hi,
> 
>          I walked through ext2 code, and found one potential NULL deference
> in ext2/xattr.c.  The version is 2.6.35-rc4, while earlier versions have the
> same problem.
>          If you configure EXT2_XATTR_DEBUG, you'll get:
> # define ea_idebug(inode, f...) do { \
>                 printk(KERN_DEBUG "inode %s:%ld: ", \
>                         inode->i_sb->s_id, inode->i_ino); \
>                 printk(f); \
>                 printk("\n"); \
>         } while (0)
> 
> In ext2/xttr.c ext2_xattr_get, NULL pointer check is done after
> ea_idebug call, so some may hit NULL deference here.
>  ext2_xattr_get(struct inode *inode, int name_index, const char *name,
>                 void *buffer, size_t buffer_size)
>  {
>          struct buffer_head *bh = NULL;
>          struct ext2_xattr_entry *entry;
>          size_t name_len, size;
>          char *end;
>          int error;
> 
>          ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
>                    name_index, name, buffer, (long)buffer_size);
> 
>          if (name == NULL)
>                  return -EINVAL;
> 
> 
> Following is my patch. Please check it.
> The patch is against kernel 2.6.35-rc4.
> 
> 
> From adc1fa6535034db3b6d8deebda6ec7eaa8bfd2f8 Mon Sep 17 00:00:00 2001
> From: Wang Sheng-Hui <crosslonelyover@gmail.com>
> Date: Sat, 10 Jul 2010 16:05:53 +0800
> Subject: [PATCH] avoid NULL deference in ext2_xattr_get
> 
> 
> Signed-off-by: Wang Sheng-Hui <crosslonelyover@gmail.com>
> ---
>  fs/ext2/xattr.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 7c39157..81ec1c6 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -156,11 +156,12 @@ ext2_xattr_get(struct inode *inode, int
> name_index, const char *name,
>  	char *end;
>  	int error;
> 
> +	if (name == NULL)
> +		return -EINVAL;
> +
>  	ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
>  		  name_index, name, buffer, (long)buffer_size);
> 
> -	if (name == NULL)
> -		return -EINVAL;
>  	down_read(&EXT2_I(inode)->xattr_sem);
>  	error = -ENODATA;
>  	if (!EXT2_I(inode)->i_file_acl)
> -- 
> 1.6.3.3
> 
> 
> 
> 
> -- 
> 
> 
> Thanks and Best Regards,
> shenghui
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] avoid NULL deference in ext2_xattr_get
@ 2010-07-10 19:10   ` David Beal
  0 siblings, 0 replies; 10+ messages in thread
From: David Beal @ 2010-07-10 19:10 UTC (permalink / raw)
  To: shenghui; +Cc: kernel-janitors, linux-kernel, linux-ext4


Hi Sheng-Hui,

There is no NULL dereference in this particular case.  In Linux's printing
routines, printk("%s",(char*)0) is safe, and would result in a "<NULL>" token
being printed.

Indicating that the "name" argument is "<NULL>" in the output of the ea_debug
routine is clearly valuable, as its purpose is to emit debugging messages.

If your patch were applied, then a log message indicating that "name" is
"<NULL>" would not appear.  Adding ternary operator inside printk() argument
body is also not necessary.

Specifically, in lib/vsnprintf.c, which services printk, a check against NULL
is made for the string format specifier, %s:

lib/vsnprintf.c:
...
static char *string(char *buf, char *end, char *s, struct printf_spec spec)
{
        int len, i;

        if ((unsigned long)s < PAGE_SIZE)
                s = "<NULL>";
...

The NULL check inside snprintf is GOOD, as it makes printk safer, and removes
the need for the thousands of conditional branches that would need to be done
OUTSIDE the printk function in order to check whether the argument to %s is
NULL.

Branch prediction has an easier time working with a branch in a function that
is more often in the cache.

Thank you,
David Beal


On Sat, Jul 10, 2010 at 04:07:28PM +0800, shenghui wrote:
> Hi,
> 
>          I walked through ext2 code, and found one potential NULL deference
> in ext2/xattr.c.  The version is 2.6.35-rc4, while earlier versions have the
> same problem.
>          If you configure EXT2_XATTR_DEBUG, you'll get:
> # define ea_idebug(inode, f...) do { \
>                 printk(KERN_DEBUG "inode %s:%ld: ", \
>                         inode->i_sb->s_id, inode->i_ino); \
>                 printk(f); \
>                 printk("\n"); \
>         } while (0)
> 
> In ext2/xttr.c ext2_xattr_get, NULL pointer check is done after
> ea_idebug call, so some may hit NULL deference here.
>  ext2_xattr_get(struct inode *inode, int name_index, const char *name,
>                 void *buffer, size_t buffer_size)
>  {
>          struct buffer_head *bh = NULL;
>          struct ext2_xattr_entry *entry;
>          size_t name_len, size;
>          char *end;
>          int error;
> 
>          ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
>                    name_index, name, buffer, (long)buffer_size);
> 
>          if (name = NULL)
>                  return -EINVAL;
> 
> 
> Following is my patch. Please check it.
> The patch is against kernel 2.6.35-rc4.
> 
> 
> From adc1fa6535034db3b6d8deebda6ec7eaa8bfd2f8 Mon Sep 17 00:00:00 2001
> From: Wang Sheng-Hui <crosslonelyover@gmail.com>
> Date: Sat, 10 Jul 2010 16:05:53 +0800
> Subject: [PATCH] avoid NULL deference in ext2_xattr_get
> 
> 
> Signed-off-by: Wang Sheng-Hui <crosslonelyover@gmail.com>
> ---
>  fs/ext2/xattr.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 7c39157..81ec1c6 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -156,11 +156,12 @@ ext2_xattr_get(struct inode *inode, int
> name_index, const char *name,
>  	char *end;
>  	int error;
> 
> +	if (name = NULL)
> +		return -EINVAL;
> +
>  	ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
>  		  name_index, name, buffer, (long)buffer_size);
> 
> -	if (name = NULL)
> -		return -EINVAL;
>  	down_read(&EXT2_I(inode)->xattr_sem);
>  	error = -ENODATA;
>  	if (!EXT2_I(inode)->i_file_acl)
> -- 
> 1.6.3.3
> 
> 
> 
> 
> -- 
> 
> 
> Thanks and Best Regards,
> shenghui
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] avoid NULL deference in ext2_xattr_get
  2010-07-10 19:10   ` David Beal
@ 2010-07-11  0:32     ` shenghui
  -1 siblings, 0 replies; 10+ messages in thread
From: shenghui @ 2010-07-11  0:32 UTC (permalink / raw)
  To: David Beal; +Cc: kernel-janitors, linux-kernel, linux-ext4

Thanks for your explanation!

-- 


Thanks and Best Regards,
shenghui

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] avoid NULL deference in ext2_xattr_get
@ 2010-07-11  0:32     ` shenghui
  0 siblings, 0 replies; 10+ messages in thread
From: shenghui @ 2010-07-11  0:32 UTC (permalink / raw)
  To: David Beal; +Cc: kernel-janitors, linux-kernel, linux-ext4

Thanks for your explanation!

-- 


Thanks and Best Regards,
shenghui

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-07-11  0:32 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-10  8:07 [PATCH] avoid NULL deference in ext2_xattr_get shenghui
2010-07-10  8:07 ` shenghui
2010-07-10  9:07 ` Dan Carpenter
2010-07-10  9:07   ` Dan Carpenter
2010-07-10  9:21 ` Dan Carpenter
2010-07-10  9:21   ` Dan Carpenter
2010-07-10 19:10 ` David Beal
2010-07-10 19:10   ` David Beal
2010-07-11  0:32   ` shenghui
2010-07-11  0:32     ` shenghui

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.