* [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c @ 2013-08-12 6:11 Li Zhong 2013-08-21 16:51 ` Rich Johnston 2013-10-18 16:28 ` Rich Johnston 0 siblings, 2 replies; 11+ messages in thread From: Li Zhong @ 2013-08-12 6:11 UTC (permalink / raw) To: xfsprogs; +Cc: Chandra Seetharaman Following is reported by coverity in bug 1061528: 187 __dirty_no_modify_ret(dirty); CID 1061528 (#1 of 1): Out-of-bounds access (OVERRUN)53. overrun-buffer-arg: Overrunning array "dinoc->di_pad" of 6 bytes by passing it to a function which accesses it at byte offset 15 using argument "16UL". 188 memset(dinoc->di_pad, 0, 16); It seems that di_pad here should be di_pad2, as sekharan pointed out. Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> --- repair/dinode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/repair/dinode.c b/repair/dinode.c index e607f0b..94bf2f8 100644 --- a/repair/dinode.c +++ b/repair/dinode.c @@ -183,9 +183,9 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) } for (i = 0; i < 16; i++) { - if (dinoc->di_pad[i] != 0) { + if (dinoc->di_pad2[i] != 0) { __dirty_no_modify_ret(dirty); - memset(dinoc->di_pad, 0, 16); + memset(dinoc->di_pad2, 0, 16); break; } } -- 1.8.1.4 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c 2013-08-12 6:11 [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c Li Zhong @ 2013-08-21 16:51 ` Rich Johnston 2013-08-23 16:38 ` Ben Myers 2013-10-18 16:28 ` Rich Johnston 1 sibling, 1 reply; 11+ messages in thread From: Rich Johnston @ 2013-08-21 16:51 UTC (permalink / raw) To: Li Zhong; +Cc: Chandra Seetharaman, xfsprogs Looks good, thanks for the patch Li Zhong. it has been committed. --Rich Reviewed-by: Rich Johnston <rjohnston@sgi.com> commit e7c05095f5baa9cd2e35a6de03d7dd9f51dd3910 Author: Li Zhong <zhong@linux.vnet.ibm.com> Date: Mon Aug 12 06:11:01 2013 +0000 xfsprogs: fix Out-of-bounds access in repair/dinode.c On 08/12/2013 01:11 AM, Li Zhong wrote: > Following is reported by coverity in bug 1061528: > > 187 __dirty_no_modify_ret(dirty); > > CID 1061528 (#1 of 1): Out-of-bounds access (OVERRUN)53. overrun-buffer-arg: Overrunning array "dinoc->di_pad" of 6 bytes by passing it to a function which accesses it at byte offset 15 using argument "16UL". > 188 memset(dinoc->di_pad, 0, 16); > > It seems that di_pad here should be di_pad2, as sekharan pointed out. > > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > --- > repair/dinode.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/repair/dinode.c b/repair/dinode.c > index e607f0b..94bf2f8 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -183,9 +183,9 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) > } > > for (i = 0; i < 16; i++) { > - if (dinoc->di_pad[i] != 0) { > + if (dinoc->di_pad2[i] != 0) { > __dirty_no_modify_ret(dirty); > - memset(dinoc->di_pad, 0, 16); > + memset(dinoc->di_pad2, 0, 16); > break; > } > } > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c 2013-08-21 16:51 ` Rich Johnston @ 2013-08-23 16:38 ` Ben Myers 2013-08-26 2:15 ` Li Zhong 2013-08-26 17:20 ` Eric Sandeen 0 siblings, 2 replies; 11+ messages in thread From: Ben Myers @ 2013-08-23 16:38 UTC (permalink / raw) To: Rich Johnston, Li Zhong; +Cc: Chandra Seetharaman, xfsprogs Hey Rich and Li Zhong, On Wed, Aug 21, 2013 at 11:51:11AM -0500, Rich Johnston wrote: > Looks good, thanks for the patch Li Zhong. it has been committed. > > --Rich > > Reviewed-by: Rich Johnston <rjohnston@sgi.com> > > commit e7c05095f5baa9cd2e35a6de03d7dd9f51dd3910 > Author: Li Zhong <zhong@linux.vnet.ibm.com> > Date: Mon Aug 12 06:11:01 2013 +0000 > > xfsprogs: fix Out-of-bounds access in repair/dinode.c > > On 08/12/2013 01:11 AM, Li Zhong wrote: > >Following is reported by coverity in bug 1061528: > > > >187 __dirty_no_modify_ret(dirty); > > > >CID 1061528 (#1 of 1): Out-of-bounds access (OVERRUN)53. overrun-buffer-arg: Overrunning array "dinoc->di_pad" of 6 bytes by passing it to a function which accesses it at byte offset 15 using argument "16UL". > >188 memset(dinoc->di_pad, 0, 16); > > > >It seems that di_pad here should be di_pad2, as sekharan pointed out. > > > >Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > >--- > > repair/dinode.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > >diff --git a/repair/dinode.c b/repair/dinode.c > >index e607f0b..94bf2f8 100644 > >--- a/repair/dinode.c > >+++ b/repair/dinode.c > >@@ -183,9 +183,9 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) > > } > > > > for (i = 0; i < 16; i++) { > >- if (dinoc->di_pad[i] != 0) { > >+ if (dinoc->di_pad2[i] != 0) { > > __dirty_no_modify_ret(dirty); > >- memset(dinoc->di_pad, 0, 16); > >+ memset(dinoc->di_pad2, 0, 16); > > break; > > } > > } We also discussed this issue a bit in this thread: http://oss.sgi.com/archives/xfs/2013-08/msg00228.html Looks like the loop itself is incorrect and should be removed, and Eric has suggested that the conditional be changed to a memcmp in case the size of the pad changes in the future. Would either of you care to spin up another patch to clean it up? Thanks, Ben _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c 2013-08-23 16:38 ` Ben Myers @ 2013-08-26 2:15 ` Li Zhong 2013-08-26 16:51 ` Ben Myers 2013-08-26 17:20 ` Eric Sandeen 1 sibling, 1 reply; 11+ messages in thread From: Li Zhong @ 2013-08-26 2:15 UTC (permalink / raw) To: Ben Myers; +Cc: Rich Johnston, Chandra Seetharaman, xfsprogs On Fri, 2013-08-23 at 11:38 -0500, Ben Myers wrote: > Hey Rich and Li Zhong, > > On Wed, Aug 21, 2013 at 11:51:11AM -0500, Rich Johnston wrote: > > Looks good, thanks for the patch Li Zhong. it has been committed. > > > > --Rich > > > > Reviewed-by: Rich Johnston <rjohnston@sgi.com> > > > > commit e7c05095f5baa9cd2e35a6de03d7dd9f51dd3910 > > Author: Li Zhong <zhong@linux.vnet.ibm.com> > > Date: Mon Aug 12 06:11:01 2013 +0000 > > > > xfsprogs: fix Out-of-bounds access in repair/dinode.c > > > > On 08/12/2013 01:11 AM, Li Zhong wrote: > > >Following is reported by coverity in bug 1061528: > > > > > >187 __dirty_no_modify_ret(dirty); > > > > > >CID 1061528 (#1 of 1): Out-of-bounds access (OVERRUN)53. overrun-buffer-arg: Overrunning array "dinoc->di_pad" of 6 bytes by passing it to a function which accesses it at byte offset 15 using argument "16UL". > > >188 memset(dinoc->di_pad, 0, 16); > > > > > >It seems that di_pad here should be di_pad2, as sekharan pointed out. > > > > > >Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > > >--- > > > repair/dinode.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > >diff --git a/repair/dinode.c b/repair/dinode.c > > >index e607f0b..94bf2f8 100644 > > >--- a/repair/dinode.c > > >+++ b/repair/dinode.c > > >@@ -183,9 +183,9 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) > > > } > > > > > > for (i = 0; i < 16; i++) { > > >- if (dinoc->di_pad[i] != 0) { > > >+ if (dinoc->di_pad2[i] != 0) { > > > __dirty_no_modify_ret(dirty); > > >- memset(dinoc->di_pad, 0, 16); > > >+ memset(dinoc->di_pad2, 0, 16); > > > break; > > > } > > > } > > We also discussed this issue a bit in this thread: > http://oss.sgi.com/archives/xfs/2013-08/msg00228.html > > Looks like the loop itself is incorrect and should be removed, and Eric has > suggested that the conditional be changed to a memcmp in case the size of the > pad changes in the future. Would either of you care to spin up another patch > to clean it up? Hi Ben, If I understand correctly, we need to change 16 to be a sizeof the di_pad2 array(like the fix attached below)? It seems to me that the loop is needed to check all of the 16 entries in the array? Thanks, Zhong --- diff --git a/repair/dinode.c b/repair/dinode.c index b2b9a95..7469fc8 100644 --- a/repair/dinode.c +++ b/repair/dinode.c @@ -182,10 +182,10 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) platform_uuid_copy(&dinoc->di_uuid, &mp->m_sb.sb_uuid); } - for (i = 0; i < 16; i++) { + for (i = 0; i < sizeof(dinoc->di_pad2)/sizeof(dinoc->di_pad2[0]); i++) { if (dinoc->di_pad2[i] != 0) { __dirty_no_modify_ret(dirty); - memset(dinoc->di_pad2, 0, 16); + memset(dinoc->di_pad2, 0, sizeof(dinoc->di_pad2)); break; } } _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c 2013-08-26 2:15 ` Li Zhong @ 2013-08-26 16:51 ` Ben Myers 2013-08-27 1:57 ` Li Zhong 0 siblings, 1 reply; 11+ messages in thread From: Ben Myers @ 2013-08-26 16:51 UTC (permalink / raw) To: Li Zhong; +Cc: Rich Johnston, Chandra Seetharaman, xfsprogs Hey Zhong, On Mon, Aug 26, 2013 at 10:15:27AM +0800, Li Zhong wrote: > On Fri, 2013-08-23 at 11:38 -0500, Ben Myers wrote: > > Hey Rich and Li Zhong, > > > > On Wed, Aug 21, 2013 at 11:51:11AM -0500, Rich Johnston wrote: > > > Looks good, thanks for the patch Li Zhong. it has been committed. > > > > > > --Rich > > > > > > Reviewed-by: Rich Johnston <rjohnston@sgi.com> > > > > > > commit e7c05095f5baa9cd2e35a6de03d7dd9f51dd3910 > > > Author: Li Zhong <zhong@linux.vnet.ibm.com> > > > Date: Mon Aug 12 06:11:01 2013 +0000 > > > > > > xfsprogs: fix Out-of-bounds access in repair/dinode.c > > > > > > On 08/12/2013 01:11 AM, Li Zhong wrote: > > > >Following is reported by coverity in bug 1061528: > > > > > > > >187 __dirty_no_modify_ret(dirty); > > > > > > > >CID 1061528 (#1 of 1): Out-of-bounds access (OVERRUN)53. overrun-buffer-arg: Overrunning array "dinoc->di_pad" of 6 bytes by passing it to a function which accesses it at byte offset 15 using argument "16UL". > > > >188 memset(dinoc->di_pad, 0, 16); > > > > > > > >It seems that di_pad here should be di_pad2, as sekharan pointed out. > > > > > > > >Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > > > >--- > > > > repair/dinode.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > >diff --git a/repair/dinode.c b/repair/dinode.c > > > >index e607f0b..94bf2f8 100644 > > > >--- a/repair/dinode.c > > > >+++ b/repair/dinode.c > > > >@@ -183,9 +183,9 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) > > > > } > > > > > > > > for (i = 0; i < 16; i++) { > > > >- if (dinoc->di_pad[i] != 0) { > > > >+ if (dinoc->di_pad2[i] != 0) { > > > > __dirty_no_modify_ret(dirty); > > > >- memset(dinoc->di_pad, 0, 16); > > > >+ memset(dinoc->di_pad2, 0, 16); > > > > break; > > > > } > > > > } > > > > We also discussed this issue a bit in this thread: > > http://oss.sgi.com/archives/xfs/2013-08/msg00228.html > > > > Looks like the loop itself is incorrect and should be removed, and Eric has > > suggested that the conditional be changed to a memcmp in case the size of the > > pad changes in the future. Would either of you care to spin up another patch > > to clean it up? > > Hi Ben, > > If I understand correctly, we need to change 16 to be a sizeof the > di_pad2 array(like the fix attached below)? > > It seems to me that the loop is needed to check all of the 16 entries in > the array? You are correct. We need the loop... > Thanks, Zhong > > --- > diff --git a/repair/dinode.c b/repair/dinode.c > index b2b9a95..7469fc8 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -182,10 +182,10 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) > platform_uuid_copy(&dinoc->di_uuid, &mp->m_sb.sb_uuid); > } > > - for (i = 0; i < 16; i++) { > + for (i = 0; i < sizeof(dinoc->di_pad2)/sizeof(dinoc->di_pad2[0]); i++) { Just sizeof(dinoc->di_pad2) would be enough here, right? No need for the division I think. > if (dinoc->di_pad2[i] != 0) { > __dirty_no_modify_ret(dirty); > - memset(dinoc->di_pad2, 0, 16); > + memset(dinoc->di_pad2, 0, sizeof(dinoc->di_pad2)); ^^^^^^^^^^^^^^^^^^^^^^ That guy will return 16, so we'll memset off the end of di_pad2. sizeof(dinoc->di_pad2[0]) would work. Thanks! -Ben _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c 2013-08-26 16:51 ` Ben Myers @ 2013-08-27 1:57 ` Li Zhong 0 siblings, 0 replies; 11+ messages in thread From: Li Zhong @ 2013-08-27 1:57 UTC (permalink / raw) To: Ben Myers; +Cc: Rich Johnston, Chandra Seetharaman, xfsprogs On Mon, 2013-08-26 at 11:51 -0500, Ben Myers wrote: > Hey Zhong, > > On Mon, Aug 26, 2013 at 10:15:27AM +0800, Li Zhong wrote: > > On Fri, 2013-08-23 at 11:38 -0500, Ben Myers wrote: > > > Hey Rich and Li Zhong, > > > > > > On Wed, Aug 21, 2013 at 11:51:11AM -0500, Rich Johnston wrote: > > > > Looks good, thanks for the patch Li Zhong. it has been committed. > > > > > > > > --Rich > > > > > > > > Reviewed-by: Rich Johnston <rjohnston@sgi.com> > > > > > > > > commit e7c05095f5baa9cd2e35a6de03d7dd9f51dd3910 > > > > Author: Li Zhong <zhong@linux.vnet.ibm.com> > > > > Date: Mon Aug 12 06:11:01 2013 +0000 > > > > > > > > xfsprogs: fix Out-of-bounds access in repair/dinode.c > > > > > > > > On 08/12/2013 01:11 AM, Li Zhong wrote: > > > > >Following is reported by coverity in bug 1061528: > > > > > > > > > >187 __dirty_no_modify_ret(dirty); > > > > > > > > > >CID 1061528 (#1 of 1): Out-of-bounds access (OVERRUN)53. overrun-buffer-arg: Overrunning array "dinoc->di_pad" of 6 bytes by passing it to a function which accesses it at byte offset 15 using argument "16UL". > > > > >188 memset(dinoc->di_pad, 0, 16); > > > > > > > > > >It seems that di_pad here should be di_pad2, as sekharan pointed out. > > > > > > > > > >Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > > > > >--- > > > > > repair/dinode.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > >diff --git a/repair/dinode.c b/repair/dinode.c > > > > >index e607f0b..94bf2f8 100644 > > > > >--- a/repair/dinode.c > > > > >+++ b/repair/dinode.c > > > > >@@ -183,9 +183,9 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) > > > > > } > > > > > > > > > > for (i = 0; i < 16; i++) { > > > > >- if (dinoc->di_pad[i] != 0) { > > > > >+ if (dinoc->di_pad2[i] != 0) { > > > > > __dirty_no_modify_ret(dirty); > > > > >- memset(dinoc->di_pad, 0, 16); > > > > >+ memset(dinoc->di_pad2, 0, 16); > > > > > break; > > > > > } > > > > > } > > > > > > We also discussed this issue a bit in this thread: > > > http://oss.sgi.com/archives/xfs/2013-08/msg00228.html > > > > > > Looks like the loop itself is incorrect and should be removed, and Eric has > > > suggested that the conditional be changed to a memcmp in case the size of the > > > pad changes in the future. Would either of you care to spin up another patch > > > to clean it up? > > > > Hi Ben, > > > > If I understand correctly, we need to change 16 to be a sizeof the > > di_pad2 array(like the fix attached below)? > > > > It seems to me that the loop is needed to check all of the 16 entries in > > the array? > > You are correct. We need the loop... > > > Thanks, Zhong > > > > --- > > diff --git a/repair/dinode.c b/repair/dinode.c > > index b2b9a95..7469fc8 100644 > > --- a/repair/dinode.c > > +++ b/repair/dinode.c > > @@ -182,10 +182,10 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) > > platform_uuid_copy(&dinoc->di_uuid, &mp->m_sb.sb_uuid); > > } > > > > - for (i = 0; i < 16; i++) { > > + for (i = 0; i < sizeof(dinoc->di_pad2)/sizeof(dinoc->di_pad2[0]); i++) { > > Just sizeof(dinoc->di_pad2) would be enough here, right? No need for the > division I think. I think the division could be used to calculate the number of elements in the array(16). But yes, in this case, as the element in the array is 1 byte in size, so it's the same whether we add the division or not. > > > if (dinoc->di_pad2[i] != 0) { > > __dirty_no_modify_ret(dirty); > > - memset(dinoc->di_pad2, 0, 16); > > + memset(dinoc->di_pad2, 0, sizeof(dinoc->di_pad2)); > ^^^^^^^^^^^^^^^^^^^^^^ > > That guy will return 16, so we'll memset off the end of di_pad2. > sizeof(dinoc->di_pad2[0]) would work. I think we need to memset the whole array, if we find some non-zero value there. Thanks, Zhong > > Thanks! > -Ben > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c 2013-08-23 16:38 ` Ben Myers 2013-08-26 2:15 ` Li Zhong @ 2013-08-26 17:20 ` Eric Sandeen 2013-08-26 17:40 ` Ben Myers 2013-08-27 1:58 ` Li Zhong 1 sibling, 2 replies; 11+ messages in thread From: Eric Sandeen @ 2013-08-26 17:20 UTC (permalink / raw) To: Ben Myers; +Cc: Rich Johnston, Chandra Seetharaman, Li Zhong, xfsprogs On 8/23/13 11:38 AM, Ben Myers wrote: > Hey Rich and Li Zhong, > > On Wed, Aug 21, 2013 at 11:51:11AM -0500, Rich Johnston wrote: >> Looks good, thanks for the patch Li Zhong. it has been committed. >> >> --Rich >> >> Reviewed-by: Rich Johnston <rjohnston@sgi.com> >> >> commit e7c05095f5baa9cd2e35a6de03d7dd9f51dd3910 >> Author: Li Zhong <zhong@linux.vnet.ibm.com> >> Date: Mon Aug 12 06:11:01 2013 +0000 >> >> xfsprogs: fix Out-of-bounds access in repair/dinode.c >> >> On 08/12/2013 01:11 AM, Li Zhong wrote: >>> Following is reported by coverity in bug 1061528: >>> >>> 187 __dirty_no_modify_ret(dirty); >>> >>> CID 1061528 (#1 of 1): Out-of-bounds access (OVERRUN)53. overrun-buffer-arg: Overrunning array "dinoc->di_pad" of 6 bytes by passing it to a function which accesses it at byte offset 15 using argument "16UL". >>> 188 memset(dinoc->di_pad, 0, 16); >>> >>> It seems that di_pad here should be di_pad2, as sekharan pointed out. >>> >>> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> >>> --- >>> repair/dinode.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/repair/dinode.c b/repair/dinode.c >>> index e607f0b..94bf2f8 100644 >>> --- a/repair/dinode.c >>> +++ b/repair/dinode.c >>> @@ -183,9 +183,9 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) >>> } >>> >>> for (i = 0; i < 16; i++) { >>> - if (dinoc->di_pad[i] != 0) { >>> + if (dinoc->di_pad2[i] != 0) { >>> __dirty_no_modify_ret(dirty); >>> - memset(dinoc->di_pad, 0, 16); >>> + memset(dinoc->di_pad2, 0, 16); >>> break; >>> } >>> } > > We also discussed this issue a bit in this thread: > http://oss.sgi.com/archives/xfs/2013-08/msg00228.html > > Looks like the loop itself is incorrect and should be removed, and Eric has > suggested that the conditional be changed to a memcmp in case the size of the > pad changes in the future. Would either of you care to spin up another patch > to clean it up? I think I was confused; it seems fine as it is in git, not sure what I was thinking. memcmp can't use a bare "0" as an arg, so it's not ideal to use either. Not a huge fan of the hard-coded 16, but I think the code is correct now; we can probably move on to real problems. ;) -Eric _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c 2013-08-26 17:20 ` Eric Sandeen @ 2013-08-26 17:40 ` Ben Myers 2013-08-27 2:01 ` Li Zhong 2013-08-27 1:58 ` Li Zhong 1 sibling, 1 reply; 11+ messages in thread From: Ben Myers @ 2013-08-26 17:40 UTC (permalink / raw) To: Eric Sandeen, Rich Johnston, Li Zhong; +Cc: Chandra Seetharaman, xfsprogs Hey, On Mon, Aug 26, 2013 at 12:20:17PM -0500, Eric Sandeen wrote: > On 8/23/13 11:38 AM, Ben Myers wrote: > > Hey Rich and Li Zhong, > > > > On Wed, Aug 21, 2013 at 11:51:11AM -0500, Rich Johnston wrote: > >> Looks good, thanks for the patch Li Zhong. it has been committed. > >> > >> --Rich > >> > >> Reviewed-by: Rich Johnston <rjohnston@sgi.com> > >> > >> commit e7c05095f5baa9cd2e35a6de03d7dd9f51dd3910 > >> Author: Li Zhong <zhong@linux.vnet.ibm.com> > >> Date: Mon Aug 12 06:11:01 2013 +0000 > >> > >> xfsprogs: fix Out-of-bounds access in repair/dinode.c > >> > >> On 08/12/2013 01:11 AM, Li Zhong wrote: > >>> Following is reported by coverity in bug 1061528: > >>> > >>> 187 __dirty_no_modify_ret(dirty); > >>> > >>> CID 1061528 (#1 of 1): Out-of-bounds access (OVERRUN)53. overrun-buffer-arg: Overrunning array "dinoc->di_pad" of 6 bytes by passing it to a function which accesses it at byte offset 15 using argument "16UL". > >>> 188 memset(dinoc->di_pad, 0, 16); > >>> > >>> It seems that di_pad here should be di_pad2, as sekharan pointed out. > >>> > >>> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > >>> --- > >>> repair/dinode.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/repair/dinode.c b/repair/dinode.c > >>> index e607f0b..94bf2f8 100644 > >>> --- a/repair/dinode.c > >>> +++ b/repair/dinode.c > >>> @@ -183,9 +183,9 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) > >>> } > >>> > >>> for (i = 0; i < 16; i++) { > >>> - if (dinoc->di_pad[i] != 0) { > >>> + if (dinoc->di_pad2[i] != 0) { > >>> __dirty_no_modify_ret(dirty); > >>> - memset(dinoc->di_pad, 0, 16); > >>> + memset(dinoc->di_pad2, 0, 16); > >>> break; > >>> } > >>> } > > > > We also discussed this issue a bit in this thread: > > http://oss.sgi.com/archives/xfs/2013-08/msg00228.html > > > > Looks like the loop itself is incorrect and should be removed, and Eric has > > suggested that the conditional be changed to a memcmp in case the size of the > > pad changes in the future. Would either of you care to spin up another patch > > to clean it up? > > I think I was confused; it seems fine as it is in git, not sure what I was > thinking. > > memcmp can't use a bare "0" as an arg, so it's not ideal to use either. > > Not a huge fan of the hard-coded 16, but I think the code is correct now; we > can probably move on to real problems. ;) 185 for (i = 0; i < 16; i++) { 186 if (dinoc->di_pad2[i] != 0) { 187 __dirty_no_modify_ret(dirty); 188 memset(dinoc->di_pad2, 0, 16); 189 break; 190 } 191 } D'oh! I was mistaken too! I was thinking that line 188 read 'memset(&dinoc->di_pad2[i], 0, 16);' and that we were going off the end of the array as i increased... Teach me to look a little closer. I agree that the current code is fine. Apologies to Rich and Zhong. -Ben _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c 2013-08-26 17:40 ` Ben Myers @ 2013-08-27 2:01 ` Li Zhong 0 siblings, 0 replies; 11+ messages in thread From: Li Zhong @ 2013-08-27 2:01 UTC (permalink / raw) To: Ben Myers; +Cc: Rich Johnston, Eric Sandeen, Chandra Seetharaman, xfsprogs On Mon, 2013-08-26 at 12:40 -0500, Ben Myers wrote: > Hey, > > On Mon, Aug 26, 2013 at 12:20:17PM -0500, Eric Sandeen wrote: > > On 8/23/13 11:38 AM, Ben Myers wrote: > > > Hey Rich and Li Zhong, > > > > > > On Wed, Aug 21, 2013 at 11:51:11AM -0500, Rich Johnston wrote: > > >> Looks good, thanks for the patch Li Zhong. it has been committed. > > >> > > >> --Rich > > >> > > >> Reviewed-by: Rich Johnston <rjohnston@sgi.com> > > >> > > >> commit e7c05095f5baa9cd2e35a6de03d7dd9f51dd3910 > > >> Author: Li Zhong <zhong@linux.vnet.ibm.com> > > >> Date: Mon Aug 12 06:11:01 2013 +0000 > > >> > > >> xfsprogs: fix Out-of-bounds access in repair/dinode.c > > >> > > >> On 08/12/2013 01:11 AM, Li Zhong wrote: > > >>> Following is reported by coverity in bug 1061528: > > >>> > > >>> 187 __dirty_no_modify_ret(dirty); > > >>> > > >>> CID 1061528 (#1 of 1): Out-of-bounds access (OVERRUN)53. overrun-buffer-arg: Overrunning array "dinoc->di_pad" of 6 bytes by passing it to a function which accesses it at byte offset 15 using argument "16UL". > > >>> 188 memset(dinoc->di_pad, 0, 16); > > >>> > > >>> It seems that di_pad here should be di_pad2, as sekharan pointed out. > > >>> > > >>> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > > >>> --- > > >>> repair/dinode.c | 4 ++-- > > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > > >>> > > >>> diff --git a/repair/dinode.c b/repair/dinode.c > > >>> index e607f0b..94bf2f8 100644 > > >>> --- a/repair/dinode.c > > >>> +++ b/repair/dinode.c > > >>> @@ -183,9 +183,9 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) > > >>> } > > >>> > > >>> for (i = 0; i < 16; i++) { > > >>> - if (dinoc->di_pad[i] != 0) { > > >>> + if (dinoc->di_pad2[i] != 0) { > > >>> __dirty_no_modify_ret(dirty); > > >>> - memset(dinoc->di_pad, 0, 16); > > >>> + memset(dinoc->di_pad2, 0, 16); > > >>> break; > > >>> } > > >>> } > > > > > > We also discussed this issue a bit in this thread: > > > http://oss.sgi.com/archives/xfs/2013-08/msg00228.html > > > > > > Looks like the loop itself is incorrect and should be removed, and Eric has > > > suggested that the conditional be changed to a memcmp in case the size of the > > > pad changes in the future. Would either of you care to spin up another patch > > > to clean it up? > > > > I think I was confused; it seems fine as it is in git, not sure what I was > > thinking. > > > > memcmp can't use a bare "0" as an arg, so it's not ideal to use either. > > > > Not a huge fan of the hard-coded 16, but I think the code is correct now; we > > can probably move on to real problems. ;) > > 185 for (i = 0; i < 16; i++) { > 186 if (dinoc->di_pad2[i] != 0) { > 187 __dirty_no_modify_ret(dirty); > 188 memset(dinoc->di_pad2, 0, 16); > 189 break; > 190 } > 191 } > > D'oh! I was mistaken too! > > I was thinking that line 188 read 'memset(&dinoc->di_pad2[i], 0, 16);' and that > we were going off the end of the array as i increased... Teach me to look a > little closer. I see, hehe > > I agree that the current code is fine. > > Apologies to Rich and Zhong. No problem, and after the discussion, maybe we could improve the code by removing the hard-coded 16 :) Thanks, Zhong > > -Ben > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c 2013-08-26 17:20 ` Eric Sandeen 2013-08-26 17:40 ` Ben Myers @ 2013-08-27 1:58 ` Li Zhong 1 sibling, 0 replies; 11+ messages in thread From: Li Zhong @ 2013-08-27 1:58 UTC (permalink / raw) To: Eric Sandeen; +Cc: Ben Myers, Rich Johnston, Chandra Seetharaman, xfsprogs On Mon, 2013-08-26 at 12:20 -0500, Eric Sandeen wrote: > On 8/23/13 11:38 AM, Ben Myers wrote: > > Hey Rich and Li Zhong, > > > > On Wed, Aug 21, 2013 at 11:51:11AM -0500, Rich Johnston wrote: > >> Looks good, thanks for the patch Li Zhong. it has been committed. > >> > >> --Rich > >> > >> Reviewed-by: Rich Johnston <rjohnston@sgi.com> > >> > >> commit e7c05095f5baa9cd2e35a6de03d7dd9f51dd3910 > >> Author: Li Zhong <zhong@linux.vnet.ibm.com> > >> Date: Mon Aug 12 06:11:01 2013 +0000 > >> > >> xfsprogs: fix Out-of-bounds access in repair/dinode.c > >> > >> On 08/12/2013 01:11 AM, Li Zhong wrote: > >>> Following is reported by coverity in bug 1061528: > >>> > >>> 187 __dirty_no_modify_ret(dirty); > >>> > >>> CID 1061528 (#1 of 1): Out-of-bounds access (OVERRUN)53. overrun-buffer-arg: Overrunning array "dinoc->di_pad" of 6 bytes by passing it to a function which accesses it at byte offset 15 using argument "16UL". > >>> 188 memset(dinoc->di_pad, 0, 16); > >>> > >>> It seems that di_pad here should be di_pad2, as sekharan pointed out. > >>> > >>> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > >>> --- > >>> repair/dinode.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/repair/dinode.c b/repair/dinode.c > >>> index e607f0b..94bf2f8 100644 > >>> --- a/repair/dinode.c > >>> +++ b/repair/dinode.c > >>> @@ -183,9 +183,9 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) > >>> } > >>> > >>> for (i = 0; i < 16; i++) { > >>> - if (dinoc->di_pad[i] != 0) { > >>> + if (dinoc->di_pad2[i] != 0) { > >>> __dirty_no_modify_ret(dirty); > >>> - memset(dinoc->di_pad, 0, 16); > >>> + memset(dinoc->di_pad2, 0, 16); > >>> break; > >>> } > >>> } > > > > We also discussed this issue a bit in this thread: > > http://oss.sgi.com/archives/xfs/2013-08/msg00228.html > > > > Looks like the loop itself is incorrect and should be removed, and Eric has > > suggested that the conditional be changed to a memcmp in case the size of the > > pad changes in the future. Would either of you care to spin up another patch > > to clean it up? > > I think I was confused; it seems fine as it is in git, not sure what I was > thinking. > > memcmp can't use a bare "0" as an arg, so it's not ideal to use either. > > Not a huge fan of the hard-coded 16, but I think the code is correct now; we > can probably move on to real problems. ;) OK :) Or maybe we could improve it with the calculation using sizeof as below(which I posted in another thread)? Thanks, Zhong --- diff --git a/repair/dinode.c b/repair/dinode.c index b2b9a95..7469fc8 100644 --- a/repair/dinode.c +++ b/repair/dinode.c @@ -182,10 +182,10 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) platform_uuid_copy(&dinoc->di_uuid, &mp->m_sb.sb_uuid); } - for (i = 0; i < 16; i++) { + for (i = 0; i < sizeof(dinoc->di_pad2)/sizeof(dinoc->di_pad2[0]); i++) { if (dinoc->di_pad2[i] != 0) { __dirty_no_modify_ret(dirty); - memset(dinoc->di_pad2, 0, 16); + memset(dinoc->di_pad2, 0, sizeof(dinoc->di_pad2)); break; } } _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c 2013-08-12 6:11 [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c Li Zhong 2013-08-21 16:51 ` Rich Johnston @ 2013-10-18 16:28 ` Rich Johnston 1 sibling, 0 replies; 11+ messages in thread From: Rich Johnston @ 2013-10-18 16:28 UTC (permalink / raw) To: Li Zhong, xfsprogs; +Cc: Chandra Seetharaman This has been committed. Thanks --Rich commit e6efb967e61a366dbe877f34e220e32866e7db42 Author: Li Zhong <zhong@linux.vnet.ibm.com> Date: Tue Aug 27 01:58:34 2013 +0000 xfsprogs: fix Out-of-bounds access in repair/dinode.c On 08/12/2013 01:11 AM, Li Zhong wrote: > Following is reported by coverity in bug 1061528: > > 187 __dirty_no_modify_ret(dirty); > > CID 1061528 (#1 of 1): Out-of-bounds access (OVERRUN)53. overrun-buffer-arg: Overrunning array "dinoc->di_pad" of 6 bytes by passing it to a function which accesses it at byte offset 15 using argument "16UL". > 188 memset(dinoc->di_pad, 0, 16); > > It seems that di_pad here should be di_pad2, as sekharan pointed out. > > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > --- > repair/dinode.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/repair/dinode.c b/repair/dinode.c > index e607f0b..94bf2f8 100644 > --- a/repair/dinode.c > +++ b/repair/dinode.c > @@ -183,9 +183,9 @@ clear_dinode_core(struct xfs_mount *mp, xfs_dinode_t *dinoc, xfs_ino_t ino_num) > } > > for (i = 0; i < 16; i++) { > - if (dinoc->di_pad[i] != 0) { > + if (dinoc->di_pad2[i] != 0) { > __dirty_no_modify_ret(dirty); > - memset(dinoc->di_pad, 0, 16); > + memset(dinoc->di_pad2, 0, 16); > break; > } > } > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-10-18 16:28 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-12 6:11 [PATCH] xfsprogs: fix Out-of-bounds access in repair/dinode.c Li Zhong 2013-08-21 16:51 ` Rich Johnston 2013-08-23 16:38 ` Ben Myers 2013-08-26 2:15 ` Li Zhong 2013-08-26 16:51 ` Ben Myers 2013-08-27 1:57 ` Li Zhong 2013-08-26 17:20 ` Eric Sandeen 2013-08-26 17:40 ` Ben Myers 2013-08-27 2:01 ` Li Zhong 2013-08-27 1:58 ` Li Zhong 2013-10-18 16:28 ` Rich Johnston
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox