Patchwork [2/2,oneiric,CVE,2/2] Change check_ruid flag to a more reasonable type

login
register
mail settings
Submitter John Johansen
Date Aug. 11, 2011, 7:39 a.m.
Message ID <1313048384-22901-3-git-send-email-john.johansen@canonical.com>
Download mbox | patch
Permalink /patch/109559/
State New
Headers show

Comments

John Johansen - Aug. 11, 2011, 7:39 a.m.
The first interation of the patch for the check ruid flag at mount time
flag returned a full uid.  However the revised patch used the check_ruid
parameter solely as a boolean flag, but missed fixing the parameters type.

Change the parameter type to int instead of uid_t.

CVE-2011-1833
BugLink: http://bugs.launchpad.net/bugs/732628

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 fs/ecryptfs/main.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
Leann Ogasawara - Aug. 11, 2011, 2:11 p.m.
Applied to Oneiric master-next and updated commit to note "UBUNTU:
SAUCE:" for now.

Thanks,
Leann

On Thu, 2011-08-11 at 00:39 -0700, John Johansen wrote:
> The first interation of the patch for the check ruid flag at mount time
> flag returned a full uid.  However the revised patch used the check_ruid
> parameter solely as a boolean flag, but missed fixing the parameters type.
> 
> Change the parameter type to int instead of uid_t.
> 
> CVE-2011-1833
> BugLink: http://bugs.launchpad.net/bugs/732628
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  fs/ecryptfs/main.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> index 703cda3..27df5b5 100644
> --- a/fs/ecryptfs/main.c
> +++ b/fs/ecryptfs/main.c
> @@ -255,7 +255,7 @@ static void ecryptfs_init_mount_crypt_stat(
>   * Returns zero on success; non-zero on error
>   */
>  static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
> -				  uid_t *check_ruid)
> +				  int *check_ruid)
>  {
>  	char *p;
>  	int rc = 0;
> @@ -484,8 +484,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>  	const char *err = "Getting sb failed";
>  	struct inode *inode;
>  	struct path path;
> -	uid_t check_ruid;
> -	int rc;
> +	int rc, check_ruid;
>  
>  	sbi = kmem_cache_zalloc(ecryptfs_sb_info_cache, GFP_KERNEL);
>  	if (!sbi) {
> -- 
> 1.7.5.4
> 
>
Tim Gardner - Aug. 11, 2011, 5:28 p.m.
I don't think this one is strictly necessary since it doesn't change 
code behavior, and needlessly diverges from upstream. A uid_t is an 
'unsigned int' which is as good as a boolean in this case.

rtg

On 08/11/2011 08:11 AM, Leann Ogasawara wrote:
> Applied to Oneiric master-next and updated commit to note "UBUNTU:
> SAUCE:" for now.
>
> Thanks,
> Leann
>
> On Thu, 2011-08-11 at 00:39 -0700, John Johansen wrote:
>> The first interation of the patch for the check ruid flag at mount time
>> flag returned a full uid.  However the revised patch used the check_ruid
>> parameter solely as a boolean flag, but missed fixing the parameters type.
>>
>> Change the parameter type to int instead of uid_t.
>>
>> CVE-2011-1833
>> BugLink: http://bugs.launchpad.net/bugs/732628
>>
>> Signed-off-by: John Johansen<john.johansen@canonical.com>
>> ---
>>   fs/ecryptfs/main.c |    5 ++---
>>   1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
>> index 703cda3..27df5b5 100644
>> --- a/fs/ecryptfs/main.c
>> +++ b/fs/ecryptfs/main.c
>> @@ -255,7 +255,7 @@ static void ecryptfs_init_mount_crypt_stat(
>>    * Returns zero on success; non-zero on error
>>    */
>>   static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
>> -				  uid_t *check_ruid)
>> +				  int *check_ruid)
>>   {
>>   	char *p;
>>   	int rc = 0;
>> @@ -484,8 +484,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
>>   	const char *err = "Getting sb failed";
>>   	struct inode *inode;
>>   	struct path path;
>> -	uid_t check_ruid;
>> -	int rc;
>> +	int rc, check_ruid;
>>
>>   	sbi = kmem_cache_zalloc(ecryptfs_sb_info_cache, GFP_KERNEL);
>>   	if (!sbi) {
>> --
>> 1.7.5.4
>>
>>
>
>
>
Leann Ogasawara - Aug. 11, 2011, 5:42 p.m.
On Thu, 2011-08-11 at 11:28 -0600, Tim Gardner wrote:
> I don't think this one is strictly necessary since it doesn't change 
> code behavior, and needlessly diverges from upstream. A uid_t is an 
> 'unsigned int' which is as good as a boolean in this case.

John, before I go yanking this from Oneiric master-next, what's the
status of this patch landing upstream?  I'd assumed since it was
associated with the CVE, that is was making it's way up and likely to
hit stable (ie, we'd be able to drop it in favor of the upstream patch
later).  As Tim has pointed out, minimal divergence from upstream is 
 the ideal scenario.

Thanks,
Leann

> On 08/11/2011 08:11 AM, Leann Ogasawara wrote:
> > Applied to Oneiric master-next and updated commit to note "UBUNTU:
> > SAUCE:" for now.
> >
> > Thanks,
> > Leann
> >
> > On Thu, 2011-08-11 at 00:39 -0700, John Johansen wrote:
> >> The first interation of the patch for the check ruid flag at mount time
> >> flag returned a full uid.  However the revised patch used the check_ruid
> >> parameter solely as a boolean flag, but missed fixing the parameters type.
> >>
> >> Change the parameter type to int instead of uid_t.
> >>
> >> CVE-2011-1833
> >> BugLink: http://bugs.launchpad.net/bugs/732628
> >>
> >> Signed-off-by: John Johansen<john.johansen@canonical.com>
> >> ---
> >>   fs/ecryptfs/main.c |    5 ++---
> >>   1 files changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> >> index 703cda3..27df5b5 100644
> >> --- a/fs/ecryptfs/main.c
> >> +++ b/fs/ecryptfs/main.c
> >> @@ -255,7 +255,7 @@ static void ecryptfs_init_mount_crypt_stat(
> >>    * Returns zero on success; non-zero on error
> >>    */
> >>   static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
> >> -				  uid_t *check_ruid)
> >> +				  int *check_ruid)
> >>   {
> >>   	char *p;
> >>   	int rc = 0;
> >> @@ -484,8 +484,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
> >>   	const char *err = "Getting sb failed";
> >>   	struct inode *inode;
> >>   	struct path path;
> >> -	uid_t check_ruid;
> >> -	int rc;
> >> +	int rc, check_ruid;
> >>
> >>   	sbi = kmem_cache_zalloc(ecryptfs_sb_info_cache, GFP_KERNEL);
> >>   	if (!sbi) {
> >> --
> >> 1.7.5.4
> >>
> >>
> >
> >
> >
> 
>
John Johansen - Aug. 11, 2011, 6:07 p.m.
On 08/11/2011 10:42 AM, Leann Ogasawara wrote:
> On Thu, 2011-08-11 at 11:28 -0600, Tim Gardner wrote:
>> I don't think this one is strictly necessary since it doesn't change
>> code behavior, and needlessly diverges from upstream. A uid_t is an
>> 'unsigned int' which is as good as a boolean in this case.
>
> John, before I go yanking this from Oneiric master-next, what's the
> status of this patch landing upstream?  I'd assumed since it was
> associated with the CVE, that is was making it's way up and likely to
> hit stable (ie, we'd be able to drop it in favor of the upstream patch
> later).  As Tim has pointed out, minimal divergence from upstream is
>   the ideal scenario.

I sent it upstream but haven't heard back from tyler on it yet.  I think
where this is going will depend on upstream.

There was a pull request for the original patch sent to linus and CC'd
to stable but last I checked it wasn't sucked in yet.

   If the original patch gets pulled in then I see this patch probably
just going to current and not stable.  However if the first patch gets
NAK'd I see the two patches combining.

I agree with tim that it isn't necessary, it is really only syntactic
sugar.  I would wait on this one, if upstream takes it we can pull it
in so we match, otherwise just ignore it.

I included mostly to document that the uid_t mistake was noticed
and the follow up patch sent.
Leann Ogasawara - Aug. 11, 2011, 6:18 p.m.
On Thu, 2011-08-11 at 11:07 -0700, John Johansen wrote:
> On 08/11/2011 10:42 AM, Leann Ogasawara wrote:
> > On Thu, 2011-08-11 at 11:28 -0600, Tim Gardner wrote:
> >> I don't think this one is strictly necessary since it doesn't change
> >> code behavior, and needlessly diverges from upstream. A uid_t is an
> >> 'unsigned int' which is as good as a boolean in this case.
> >
> > John, before I go yanking this from Oneiric master-next, what's the
> > status of this patch landing upstream?  I'd assumed since it was
> > associated with the CVE, that is was making it's way up and likely to
> > hit stable (ie, we'd be able to drop it in favor of the upstream patch
> > later).  As Tim has pointed out, minimal divergence from upstream is
> >   the ideal scenario.
> 
> I sent it upstream but haven't heard back from tyler on it yet.  I think
> where this is going will depend on upstream.
> 
> There was a pull request for the original patch sent to linus and CC'd
> to stable but last I checked it wasn't sucked in yet.
> 
>    If the original patch gets pulled in then I see this patch probably
> just going to current and not stable.  However if the first patch gets
> NAK'd I see the two patches combining.
> 
> I agree with tim that it isn't necessary, it is really only syntactic
> sugar.  I would wait on this one, if upstream takes it we can pull it
> in so we match, otherwise just ignore it.
> 
> I included mostly to document that the uid_t mistake was noticed
> and the follow up patch sent.

Thanks for the feedback.  I'll go ahead and drop from Oneiric
master-next for now.

Thanks,
Leann

Patch

diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index 703cda3..27df5b5 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -255,7 +255,7 @@  static void ecryptfs_init_mount_crypt_stat(
  * Returns zero on success; non-zero on error
  */
 static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options,
-				  uid_t *check_ruid)
+				  int *check_ruid)
 {
 	char *p;
 	int rc = 0;
@@ -484,8 +484,7 @@  static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
 	const char *err = "Getting sb failed";
 	struct inode *inode;
 	struct path path;
-	uid_t check_ruid;
-	int rc;
+	int rc, check_ruid;
 
 	sbi = kmem_cache_zalloc(ecryptfs_sb_info_cache, GFP_KERNEL);
 	if (!sbi) {