* [RESEND PATCH] autofs4 - remove string terminator check @ 2008-10-28 1:14 Ian Kent 2008-10-28 1:54 ` Andrew Morton 0 siblings, 1 reply; 5+ messages in thread From: Ian Kent @ 2008-10-28 1:14 UTC (permalink / raw) To: Andrew Morton; +Cc: autofs mailing list, Kernel Mailing List, linux-fsdevel In a previous patch a comment was made that checking for the existence of a NULL terminator in strings copied from userspace wasn't needed as this is done in many places in the kernel without problem. This patch removes this string terminator check. Signed-off-by: Ian Kent <raven@themaw.net> Signed-off-by: Jeff Moyer <jmoyer@redhat.com> --- fs/autofs4/dev-ioctl.c | 20 -------------------- 1 files changed, 0 insertions(+), 20 deletions(-) diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c index 625abf5..304c1ff 100644 --- a/fs/autofs4/dev-ioctl.c +++ b/fs/autofs4/dev-ioctl.c @@ -51,18 +51,6 @@ static int check_name(const char *name) } /* - * Check a string doesn't overrun the chunk of - * memory we copied from user land. - */ -static int invalid_str(char *str, void *end) -{ - while ((void *) str <= end) - if (!*str++) - return 0; - return -EINVAL; -} - -/* * Check that the user compiled against correct version of autofs * misc device code. * @@ -143,14 +131,6 @@ static int validate_dev_ioctl(int cmd, struct autofs_dev_ioctl *param) cmd); goto out; } - - err = invalid_str(param->path, - (void *) ((size_t) param + param->size)); - if (err) { - AUTOFS_WARN("invalid path supplied for cmd(0x%08x)", - cmd); - goto out; - } } err = 0; ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH] autofs4 - remove string terminator check 2008-10-28 1:14 [RESEND PATCH] autofs4 - remove string terminator check Ian Kent @ 2008-10-28 1:54 ` Andrew Morton 2008-10-28 2:07 ` Ian Kent 0 siblings, 1 reply; 5+ messages in thread From: Andrew Morton @ 2008-10-28 1:54 UTC (permalink / raw) To: Ian Kent; +Cc: autofs mailing list, Kernel Mailing List, linux-fsdevel On Tue, 28 Oct 2008 10:14:30 +0900 Ian Kent <raven@themaw.net> wrote: > In a previous patch a comment was made that checking for the existence of > a NULL terminator in strings copied from userspace wasn't needed as this > is done in many places in the kernel without problem. This patch removes > this string terminator check. > ah, OK. Now I'm worried. > > fs/autofs4/dev-ioctl.c | 20 -------------------- > 1 files changed, 0 insertions(+), 20 deletions(-) > > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c > index 625abf5..304c1ff 100644 > --- a/fs/autofs4/dev-ioctl.c > +++ b/fs/autofs4/dev-ioctl.c > @@ -51,18 +51,6 @@ static int check_name(const char *name) > } > > /* > - * Check a string doesn't overrun the chunk of > - * memory we copied from user land. > - */ > -static int invalid_str(char *str, void *end) > -{ > - while ((void *) str <= end) > - if (!*str++) > - return 0; > - return -EINVAL; > -} > - > -/* > * Check that the user compiled against correct version of autofs > * misc device code. > * > @@ -143,14 +131,6 @@ static int validate_dev_ioctl(int cmd, struct autofs_dev_ioctl *param) > cmd); > goto out; > } > - > - err = invalid_str(param->path, > - (void *) ((size_t) param + param->size)); > - if (err) { > - AUTOFS_WARN("invalid path supplied for cmd(0x%08x)", > - cmd); > - goto out; > - } > } > > err = 0; What will now happen if userspace passes in a non-null-terminated string (if that's possible)? Presumably that isn't possible, or it's never been tested, because before we check for null-termination we run check_name(), which _assumes_ null-termination! The comment over validate_dev_ioctl() will need the "and is terminated" removed after this change, yes? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH] autofs4 - remove string terminator check 2008-10-28 1:54 ` Andrew Morton @ 2008-10-28 2:07 ` Ian Kent 2008-10-28 2:11 ` Ian Kent 0 siblings, 1 reply; 5+ messages in thread From: Ian Kent @ 2008-10-28 2:07 UTC (permalink / raw) To: Andrew Morton; +Cc: autofs mailing list, Kernel Mailing List, linux-fsdevel On Mon, 2008-10-27 at 18:54 -0700, Andrew Morton wrote: > On Tue, 28 Oct 2008 10:14:30 +0900 Ian Kent <raven@themaw.net> wrote: > > > In a previous patch a comment was made that checking for the existence of > > a NULL terminator in strings copied from userspace wasn't needed as this > > is done in many places in the kernel without problem. This patch removes > > this string terminator check. > > > > ah, OK. Now I'm worried. > > > > > fs/autofs4/dev-ioctl.c | 20 -------------------- > > 1 files changed, 0 insertions(+), 20 deletions(-) > > > > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c > > index 625abf5..304c1ff 100644 > > --- a/fs/autofs4/dev-ioctl.c > > +++ b/fs/autofs4/dev-ioctl.c > > @@ -51,18 +51,6 @@ static int check_name(const char *name) > > } > > > > /* > > - * Check a string doesn't overrun the chunk of > > - * memory we copied from user land. > > - */ > > -static int invalid_str(char *str, void *end) > > -{ > > - while ((void *) str <= end) > > - if (!*str++) > > - return 0; > > - return -EINVAL; > > -} > > - > > -/* > > * Check that the user compiled against correct version of autofs > > * misc device code. > > * > > @@ -143,14 +131,6 @@ static int validate_dev_ioctl(int cmd, struct autofs_dev_ioctl *param) > > cmd); > > goto out; > > } > > - > > - err = invalid_str(param->path, > > - (void *) ((size_t) param + param->size)); > > - if (err) { > > - AUTOFS_WARN("invalid path supplied for cmd(0x%08x)", > > - cmd); > > - goto out; > > - } > > } > > > > err = 0; > > What will now happen if userspace passes in a non-null-terminated > string (if that's possible)? > > Presumably that isn't possible, or it's never been tested, because > before we check for null-termination we run check_name(), which > _assumes_ null-termination! > > The comment over validate_dev_ioctl() will need the "and is terminated" > removed after this change, yes? Yes, but now I think I shouldn't have removed it. For my part I would have been happy to keep this and now I think I should so let's drop this patch. Ian ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH] autofs4 - remove string terminator check 2008-10-28 2:07 ` Ian Kent @ 2008-10-28 2:11 ` Ian Kent 2008-10-28 2:35 ` Ian Kent 0 siblings, 1 reply; 5+ messages in thread From: Ian Kent @ 2008-10-28 2:11 UTC (permalink / raw) To: Andrew Morton; +Cc: autofs mailing list, Kernel Mailing List, linux-fsdevel On Tue, 2008-10-28 at 11:07 +0900, Ian Kent wrote: > On Mon, 2008-10-27 at 18:54 -0700, Andrew Morton wrote: > > On Tue, 28 Oct 2008 10:14:30 +0900 Ian Kent <raven@themaw.net> wrote: > > > > > In a previous patch a comment was made that checking for the existence of > > > a NULL terminator in strings copied from userspace wasn't needed as this > > > is done in many places in the kernel without problem. This patch removes > > > this string terminator check. > > > > > > > ah, OK. Now I'm worried. > > > > > > > > fs/autofs4/dev-ioctl.c | 20 -------------------- > > > 1 files changed, 0 insertions(+), 20 deletions(-) > > > > > > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c > > > index 625abf5..304c1ff 100644 > > > --- a/fs/autofs4/dev-ioctl.c > > > +++ b/fs/autofs4/dev-ioctl.c > > > @@ -51,18 +51,6 @@ static int check_name(const char *name) > > > } > > > > > > /* > > > - * Check a string doesn't overrun the chunk of > > > - * memory we copied from user land. > > > - */ > > > -static int invalid_str(char *str, void *end) > > > -{ > > > - while ((void *) str <= end) > > > - if (!*str++) > > > - return 0; > > > - return -EINVAL; > > > -} > > > - > > > -/* > > > * Check that the user compiled against correct version of autofs > > > * misc device code. > > > * > > > @@ -143,14 +131,6 @@ static int validate_dev_ioctl(int cmd, struct autofs_dev_ioctl *param) > > > cmd); > > > goto out; > > > } > > > - > > > - err = invalid_str(param->path, > > > - (void *) ((size_t) param + param->size)); > > > - if (err) { > > > - AUTOFS_WARN("invalid path supplied for cmd(0x%08x)", > > > - cmd); > > > - goto out; > > > - } > > > } > > > > > > err = 0; > > > > What will now happen if userspace passes in a non-null-terminated > > string (if that's possible)? > > > > Presumably that isn't possible, or it's never been tested, because > > before we check for null-termination we run check_name(), which > > _assumes_ null-termination! Oh .. hang on, point taken. > > > > The comment over validate_dev_ioctl() will need the "and is terminated" > > removed after this change, yes? > > Yes, but now I think I shouldn't have removed it. > For my part I would have been happy to keep this and now I think I > should so let's drop this patch. > > Ian > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH] autofs4 - remove string terminator check 2008-10-28 2:11 ` Ian Kent @ 2008-10-28 2:35 ` Ian Kent 0 siblings, 0 replies; 5+ messages in thread From: Ian Kent @ 2008-10-28 2:35 UTC (permalink / raw) To: Andrew Morton; +Cc: autofs mailing list, Kernel Mailing List, linux-fsdevel On Tue, 2008-10-28 at 11:11 +0900, Ian Kent wrote: > On Tue, 2008-10-28 at 11:07 +0900, Ian Kent wrote: > > On Mon, 2008-10-27 at 18:54 -0700, Andrew Morton wrote: > > > On Tue, 28 Oct 2008 10:14:30 +0900 Ian Kent <raven@themaw.net> wrote: > > > > > > > In a previous patch a comment was made that checking for the existence of > > > > a NULL terminator in strings copied from userspace wasn't needed as this > > > > is done in many places in the kernel without problem. This patch removes > > > > this string terminator check. > > > > > > > > > > ah, OK. Now I'm worried. > > > > > > > > > > > fs/autofs4/dev-ioctl.c | 20 -------------------- > > > > 1 files changed, 0 insertions(+), 20 deletions(-) > > > > > > > > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c > > > > index 625abf5..304c1ff 100644 > > > > --- a/fs/autofs4/dev-ioctl.c > > > > +++ b/fs/autofs4/dev-ioctl.c > > > > @@ -51,18 +51,6 @@ static int check_name(const char *name) > > > > } > > > > > > > > /* > > > > - * Check a string doesn't overrun the chunk of > > > > - * memory we copied from user land. > > > > - */ > > > > -static int invalid_str(char *str, void *end) > > > > -{ > > > > - while ((void *) str <= end) > > > > - if (!*str++) > > > > - return 0; > > > > - return -EINVAL; > > > > -} > > > > - > > > > -/* > > > > * Check that the user compiled against correct version of autofs > > > > * misc device code. > > > > * > > > > @@ -143,14 +131,6 @@ static int validate_dev_ioctl(int cmd, struct autofs_dev_ioctl *param) > > > > cmd); > > > > goto out; > > > > } > > > > - > > > > - err = invalid_str(param->path, > > > > - (void *) ((size_t) param + param->size)); > > > > - if (err) { > > > > - AUTOFS_WARN("invalid path supplied for cmd(0x%08x)", > > > > - cmd); > > > > - goto out; > > > > - } > > > > } > > > > > > > > err = 0; > > > > > > What will now happen if userspace passes in a non-null-terminated > > > string (if that's possible)? > > > > > > Presumably that isn't possible, or it's never been tested, because > > > before we check for null-termination we run check_name(), which > > > _assumes_ null-termination! > > Oh .. hang on, point taken. > > > > > > > The comment over validate_dev_ioctl() will need the "and is terminated" > > > removed after this change, yes? > > > > Yes, but now I think I shouldn't have removed it. > > For my part I would have been happy to keep this and now I think I > > should so let's drop this patch. Just to confirm. Please drop this patch and I'll send a patch to fix the check order. > > > > Ian > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-10-28 2:35 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-10-28 1:14 [RESEND PATCH] autofs4 - remove string terminator check Ian Kent 2008-10-28 1:54 ` Andrew Morton 2008-10-28 2:07 ` Ian Kent 2008-10-28 2:11 ` Ian Kent 2008-10-28 2:35 ` Ian Kent
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).