| Message ID | 1313048384-22901-3-git-send-email-john.johansen@canonical.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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 >> >> > > >
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 > >> > >> > > > > > > > >
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.
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
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) {
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(-)