On Thu, 19 Apr 2012 10:32:43 +0530
Suresh Jayaraman <sjayaraman@xxxxxxxx> wrote:
> On 04/19/2012 07:20 AM, Jeff Layton wrote:
> > ...and add -D_FORTIFY_SOURCE=2 to the default $CFLAGS.
> >
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxx>
> > ---
> > Makefile.am | 2 +-
> > mount.cifs.c | 12 +++++++-----
> > mtab.c | 4 +++-
> > 3 files changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index d95142a..05729ca 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -1,4 +1,4 @@
> > -AM_CFLAGS = -Wall -Wextra -Werror
> > +AM_CFLAGS = -Wall -Wextra -Werror -D_FORTIFY_SOURCE=2
>
> Seems a good thing to do given that the number of vulnerability reports
> in the past.
>
Most of the vulnerabilities have occurred when people install this as a
setuid root program, and then exploit the behaviors that were designed
in from the beginning. We haven't had many (any?) vulnerabilities from
straightforward bugs...
Still, it certainly doesn't hurt...
> > ACLOCAL_AMFLAGS = -I aclocal
> >
> > root_sbindir = $(ROOTSBINDIR)
> > diff --git a/mount.cifs.c b/mount.cifs.c
> > index f0b073e..ecbf034 100644
> > --- a/mount.cifs.c
> > +++ b/mount.cifs.c
> > @@ -928,10 +928,10 @@ parse_options(const char *data, struct parsed_mount_info *parsed_info)
> > }
> > } else {
> > /* domain/username%password */
> > - const int max = MAX_DOMAIN_SIZE +
> > - MAX_USERNAME_SIZE +
> > - MOUNT_PASSWD_SIZE + 2;
> > - if (strnlen(value, max + 1) >= max + 1) {
> > + const size_t max = MAX_DOMAIN_SIZE +
> > + MAX_USERNAME_SIZE +
> > + MOUNT_PASSWD_SIZE + 2 + 1;
> > + if (strnlen(value, max) >= max) {
> > fprintf(stderr, "username too long\n");
> > return EX_USAGE;
> > }
> > @@ -1603,8 +1603,10 @@ add_mtab(char *devname, char *mountpoint, unsigned long flags, const char *fstyp
> > mountent.mnt_passno = 0;
> > rc = addmntent(pmntfile, &mountent);
> > if (rc) {
> > + int ignore __attribute__((unused));
> > +
> > fprintf(stderr, "unable to add mount entry to mtab\n");
> > - ftruncate(fd, statbuf.st_size);
> > + ignore = ftruncate(fd, statbuf.st_size);
>
> Though this would mean a little extra code (esp. with -Werror), I think
> it makes the code readable.
>
That's necessary due to the "ignored retval" warning. We could also
wrap it inside an "if() {}" block or something, but I think this is
clearer and this isn't a terribly hot codepath anyway.
> > rc = EX_FILEIO;
> > }
> > tmprc = my_endmntent(pmntfile, statbuf.st_size);
> > diff --git a/mtab.c b/mtab.c
> > index de545b7..3d42ac0 100644
> > --- a/mtab.c
> > +++ b/mtab.c
> > @@ -271,8 +271,10 @@ my_endmntent(FILE *stream, off_t size)
> >
> > /* truncate file back to "size" -- best effort here */
> > if (rc) {
> > + int ignore __attribute__((unused));
> > +
> > rc = errno;
> > - ftruncate(fd, size);
> > + ignore = ftruncate(fd, size);
> > }
> >
> > endmntent(stream);
>
> Looks good to me.
>
> Acked-by: Suresh Jayaraman <sjayaraman@xxxxxxxx>
Thanks for reviewing!
--
Jeff Layton <jlayton@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
