| Message ID | 20190320193542.GC1183@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfsprogs-5.0: fix various problems | expand |
On 3/20/19 2:35 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Fix a number of complaints about feeding sizeof(dest) directly to > strncpy. We do this by declaring the char arrays to be one larger > than necessary and subtracting one, to ensure that we never overfill > the buffer. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > mkfs/xfs_mkfs.c | 13 +++++++++++-- > quota/edit.c | 9 ++++++--- > 2 files changed, 17 insertions(+), 5 deletions(-) > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > index 9e1c6ec5..e87c692c 100644 > --- a/mkfs/xfs_mkfs.c > +++ b/mkfs/xfs_mkfs.c > @@ -3251,8 +3251,17 @@ finish_superblock_setup( > struct xfs_mount *mp, > struct xfs_sb *sbp) > { > - if (cfg->label) > - strncpy(sbp->sb_fname, cfg->label, sizeof(sbp->sb_fname)); > + if (cfg->label) { > + size_t label_len; > + > + /* > + * Labels are null terminated unless the string fits exactly > + * in the label field, so assume sb_fname is zeroed and then > + * do a memcpy because the destination isn't a normal C string. > + */ > + label_len = min(sizeof(sbp->sb_fname), strlen(cfg->label)); Heh, comparing this to the patch I wrote in <checks notes> December and never sent *cough*, it seems like it might be wise to memset the whole thing to 0, to be sure we're not leaking bytes here and there in a short label? + memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname)); Although, I suppose the prior strncpy had the same issue... *shrug* > + memcpy(sbp->sb_fname, cfg->label, label_len); > + } > > sbp->sb_dblocks = cfg->dblocks; > sbp->sb_rblocks = cfg->rtblocks; > diff --git a/quota/edit.c b/quota/edit.c > index b10a5b34..f9938b8a 100644 > --- a/quota/edit.c > +++ b/quota/edit.c > @@ -368,8 +368,7 @@ restore_file( > uint type) > { > char buffer[512]; > - char devbuffer[512]; > - char *dev = NULL; > + char dev[512]; > uint mask; > int cnt; > uint32_t id; > @@ -377,7 +376,11 @@ restore_file( > > while (fgets(buffer, sizeof(buffer), fp) != NULL) { > if (strncmp("fs = ", buffer, 5) == 0) { > - dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer)); > + /* > + * Copy the device name to dev, strip off the trailing > + * newline, and move on to the next line. > + */ > + strncpy(dev, buffer + 5, sizeof(dev) - 1); > dev[strlen(dev) - 1] = '\0'; > continue; > } Ok. I had something in io/label.c: label_f() as well. Do you not get complaints about strncpy(label, optarg, sizeof(label)); ? I essentially did the same thing as for mkfs, though with my gratuitous memset: break; case 's': - strncpy(label, optarg, sizeof(label)); + len = min(strlen(optarg), sizeof(label)); + /* Kernel doesn't care about null termination */ + memset(label, 0, sizeof(label)); + memcpy(label, optarg, len); break; default:
On Thu, Apr 04, 2019 at 04:30:38PM -0500, Eric Sandeen wrote: > On 3/20/19 2:35 PM, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Fix a number of complaints about feeding sizeof(dest) directly to > > strncpy. We do this by declaring the char arrays to be one larger > > than necessary and subtracting one, to ensure that we never overfill > > the buffer. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > mkfs/xfs_mkfs.c | 13 +++++++++++-- > > quota/edit.c | 9 ++++++--- > > 2 files changed, 17 insertions(+), 5 deletions(-) > > > > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c > > index 9e1c6ec5..e87c692c 100644 > > --- a/mkfs/xfs_mkfs.c > > +++ b/mkfs/xfs_mkfs.c > > @@ -3251,8 +3251,17 @@ finish_superblock_setup( > > struct xfs_mount *mp, > > struct xfs_sb *sbp) > > { > > - if (cfg->label) > > - strncpy(sbp->sb_fname, cfg->label, sizeof(sbp->sb_fname)); > > + if (cfg->label) { > > + size_t label_len; > > + > > + /* > > + * Labels are null terminated unless the string fits exactly > > + * in the label field, so assume sb_fname is zeroed and then > > + * do a memcpy because the destination isn't a normal C string. > > + */ > > + label_len = min(sizeof(sbp->sb_fname), strlen(cfg->label)); > > Heh, comparing this to the patch I wrote in <checks notes> December and never > sent *cough*, it seems like it might be wise to memset the whole thing to 0, > to be sure we're not leaking bytes here and there in a short label? > > + memset(sbp->sb_fname, 0, sizeof(sbp->sb_fname)); > > Although, I suppose the prior strncpy had the same issue... *shrug* I think the initialization is fine because... finish_superblock_setup sets fields in *sbp. It gets *sbp from the top of main(), which does: struct xfs_mount mbuf = {}; struct xfs_mount *mp = &mbuf; struct xfs_sb *sbp = &mp->m_sb; So assuming the empty mbuf initializer causes the whole stack variable to be initialized to zero, we don't need to do a memset here. > > > + memcpy(sbp->sb_fname, cfg->label, label_len); > > + } > > > > sbp->sb_dblocks = cfg->dblocks; > > sbp->sb_rblocks = cfg->rtblocks; > > diff --git a/quota/edit.c b/quota/edit.c > > index b10a5b34..f9938b8a 100644 > > --- a/quota/edit.c > > +++ b/quota/edit.c > > @@ -368,8 +368,7 @@ restore_file( > > uint type) > > { > > char buffer[512]; > > - char devbuffer[512]; > > - char *dev = NULL; > > + char dev[512]; > > uint mask; > > int cnt; > > uint32_t id; > > @@ -377,7 +376,11 @@ restore_file( > > > > while (fgets(buffer, sizeof(buffer), fp) != NULL) { > > if (strncmp("fs = ", buffer, 5) == 0) { > > - dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer)); > > + /* > > + * Copy the device name to dev, strip off the trailing > > + * newline, and move on to the next line. > > + */ > > + strncpy(dev, buffer + 5, sizeof(dev) - 1); > > dev[strlen(dev) - 1] = '\0'; > > continue; > > } > > Ok. > > I had something in io/label.c: label_f() as well. Do you not get complaints > about strncpy(label, optarg, sizeof(label)); ? I do, that's why I changed it...? io/label.c got changed to shut up the warning about the sizeof(firstarg) as the third parameter. > > I essentially did the same thing as for mkfs, though with my gratuitous > memset: > > break; > case 's': > - strncpy(label, optarg, sizeof(label)); > + len = min(strlen(optarg), sizeof(label)); > + /* Kernel doesn't care about null termination */ > + memset(label, 0, sizeof(label)); > + memcpy(label, optarg, len); > break; > default: > (FWIW I'm not sure I remember what we were talking about here.) Anyway, maybe I'll just resend the remaining patches as a rollup since this series is already huge. --D
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c index 9e1c6ec5..e87c692c 100644 --- a/mkfs/xfs_mkfs.c +++ b/mkfs/xfs_mkfs.c @@ -3251,8 +3251,17 @@ finish_superblock_setup( struct xfs_mount *mp, struct xfs_sb *sbp) { - if (cfg->label) - strncpy(sbp->sb_fname, cfg->label, sizeof(sbp->sb_fname)); + if (cfg->label) { + size_t label_len; + + /* + * Labels are null terminated unless the string fits exactly + * in the label field, so assume sb_fname is zeroed and then + * do a memcpy because the destination isn't a normal C string. + */ + label_len = min(sizeof(sbp->sb_fname), strlen(cfg->label)); + memcpy(sbp->sb_fname, cfg->label, label_len); + } sbp->sb_dblocks = cfg->dblocks; sbp->sb_rblocks = cfg->rtblocks; diff --git a/quota/edit.c b/quota/edit.c index b10a5b34..f9938b8a 100644 --- a/quota/edit.c +++ b/quota/edit.c @@ -368,8 +368,7 @@ restore_file( uint type) { char buffer[512]; - char devbuffer[512]; - char *dev = NULL; + char dev[512]; uint mask; int cnt; uint32_t id; @@ -377,7 +376,11 @@ restore_file( while (fgets(buffer, sizeof(buffer), fp) != NULL) { if (strncmp("fs = ", buffer, 5) == 0) { - dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer)); + /* + * Copy the device name to dev, strip off the trailing + * newline, and move on to the next line. + */ + strncpy(dev, buffer + 5, sizeof(dev) - 1); dev[strlen(dev) - 1] = '\0'; continue; }