Patchwork [02/22] sheepdog: avoid a few buffer overruns

login
register
mail settings
Submitter Jim Meyering
Date May 9, 2012, 9:23 a.m.
Message ID <1336555446-20180-3-git-send-email-jim@meyering.net>
Download mbox | patch
Permalink /patch/157980/
State New
Headers show

Comments

Jim Meyering - May 9, 2012, 9:23 a.m.
From: Jim Meyering <meyering@redhat.com>

* parse_vdiname: Use pstrcpy, not strncpy, when the destination
buffer must be NUL-terminated.
* sd_open: Likewise, avoid buffer overrun.
* do_sd_create: Likewise.  Leave the preceding memset, since
pstrcpy does not NUL-fill, and filename needs that.
* sd_snapshot_create: Add a comment/question.
* find_vdi_name: Remove a useless memset.
* sd_snapshot_goto: Remove a useless memset.
Use pstrcpy to NUL-terminate, because find_vdi_name requires
that its vdi arg (filename parameter) be NUL-terminated.
It seems ok not to NUL-fill the buffer.
Do the same for snapid: remove useless memset-0 (instead,
zero tag[0]).  Use pstrcpy, not strncpy.
* sd_snapshot_list: Use pstrcpy, not strncpy to write
into the ->name member.  Each must be NUL-terminated.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 block/sheepdog.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)
MORITA Kazutaka - May 9, 2012, 5:29 p.m.
At Wed, 09 May 2012 11:46:10 +0200,
Kevin Wolf wrote:
> 
> Am 09.05.2012 11:23, schrieb Jim Meyering:
> > From: Jim Meyering <meyering@redhat.com>
> > 
> > * parse_vdiname: Use pstrcpy, not strncpy, when the destination
> > buffer must be NUL-terminated.
> > * sd_open: Likewise, avoid buffer overrun.
> > * do_sd_create: Likewise.  Leave the preceding memset, since
> > pstrcpy does not NUL-fill, and filename needs that.
> > * sd_snapshot_create: Add a comment/question.
> > * find_vdi_name: Remove a useless memset.
> > * sd_snapshot_goto: Remove a useless memset.
> > Use pstrcpy to NUL-terminate, because find_vdi_name requires
> > that its vdi arg (filename parameter) be NUL-terminated.
> > It seems ok not to NUL-fill the buffer.
> > Do the same for snapid: remove useless memset-0 (instead,
> > zero tag[0]).  Use pstrcpy, not strncpy.
> > * sd_snapshot_list: Use pstrcpy, not strncpy to write
> > into the ->name member.  Each must be NUL-terminated.
> > 
> > Signed-off-by: Jim Meyering <meyering@redhat.com>
> 
> Acked-by: Kevin Wolf <kwolf@redhat.com>
> 
> Kazutaka, can you have a look? I think you may want to send patches on
> top to improve the places where Jim just put a comment.

Acked-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

I'll send a patch on top of this to address Jim's comments.

Kazutaka

> 
> Kevin
> 
> > ---
> >  block/sheepdog.c | 34 ++++++++++++++++++++++------------
> >  1 file changed, 22 insertions(+), 12 deletions(-)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index 0ed6b19..f2579da 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -852,14 +852,14 @@ static int parse_vdiname(BDRVSheepdogState *s, const char *filename,
> >          s->port = 0;
> >      }
> > 
> > -    strncpy(vdi, p, SD_MAX_VDI_LEN);
> > +    pstrcpy(vdi, SD_MAX_VDI_LEN, p);
> > 
> >      p = strchr(vdi, ':');
> >      if (p) {
> >          *p++ = '\0';
> >          *snapid = strtoul(p, NULL, 10);
> >          if (*snapid == 0) {
> > -            strncpy(tag, p, SD_MAX_VDI_TAG_LEN);
> > +            pstrcpy(tag, SD_MAX_VDI_TAG_LEN, p);
> >          }
> >      } else {
> >          *snapid = CURRENT_VDI_ID; /* search current vdi */
> > @@ -886,7 +886,10 @@ static int find_vdi_name(BDRVSheepdogState *s, char *filename, uint32_t snapid,
> >          return -1;
> >      }
> > 
> > -    memset(buf, 0, sizeof(buf));
> > +    /* This pair of strncpy calls ensures that the buffer is zero-filled,
> > +     * which is desirable since we'll soon be sending those bytes, and
> > +     * don't want the send_req to read uninitialized data.
> > +     */
> >      strncpy(buf, filename, SD_MAX_VDI_LEN);
> >      strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
> > 
> > @@ -1129,7 +1132,7 @@ static int sd_open(BlockDriverState *bs, const char *filename, int flags)
> >      s->max_dirty_data_idx = 0;
> > 
> >      bs->total_sectors = s->inode.vdi_size / SECTOR_SIZE;
> > -    strncpy(s->name, vdi, sizeof(s->name));
> > +    pstrcpy(s->name, sizeof(s->name), vdi);
> >      qemu_co_mutex_init(&s->lock);
> >      g_free(buf);
> >      return 0;
> > @@ -1157,8 +1160,11 @@ static int do_sd_create(char *filename, int64_t vdi_size,
> >          return -EIO;
> >      }
> > 
> > +    /* FIXME: would it be better to fail (e.g., return -EIO) when filename
> > +     * does not fit in buf?  For now, just truncate and avoid buffer overrun.
> > +     */
> >      memset(buf, 0, sizeof(buf));
> > -    strncpy(buf, filename, SD_MAX_VDI_LEN);
> > +    pstrcpy(buf, sizeof(buf), filename);
> > 
> >      memset(&hdr, 0, sizeof(hdr));
> >      hdr.opcode = SD_OP_NEW_VDI;
> > @@ -1709,6 +1715,9 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
> > 
> >      s->inode.vm_state_size = sn_info->vm_state_size;
> >      s->inode.vm_clock_nsec = sn_info->vm_clock_nsec;
> > +    /* It appears that inode.tag does not require a NUL terminator,
> > +     * which means this use of strncpy is ok.
> > +     */
> >      strncpy(s->inode.tag, sn_info->name, sizeof(s->inode.tag));
> >      /* we don't need to update entire object */
> >      datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id);
> > @@ -1771,13 +1780,13 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> > 
> >      memcpy(old_s, s, sizeof(BDRVSheepdogState));
> > 
> > -    memset(vdi, 0, sizeof(vdi));
> > -    strncpy(vdi, s->name, sizeof(vdi));
> > +    pstrcpy(vdi, sizeof(vdi), s->name);
> > 
> > -    memset(tag, 0, sizeof(tag));
> >      snapid = strtoul(snapshot_id, NULL, 10);
> > -    if (!snapid) {
> > -        strncpy(tag, s->name, sizeof(tag));
> > +    if (snapid) {
> > +        tag[0] = 0;
> > +    } else {
> > +        pstrcpy(tag, sizeof(tag), s->name);
> >      }
> > 
> >      ret = find_vdi_name(s, vdi, snapid, tag, &vid, 1);
> > @@ -1905,8 +1914,9 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
> > 
> >              snprintf(sn_tab[found].id_str, sizeof(sn_tab[found].id_str), "%u",
> >                       inode.snap_id);
> > -            strncpy(sn_tab[found].name, inode.tag,
> > -                    MIN(sizeof(sn_tab[found].name), sizeof(inode.tag)));
> > +            pstrcpy(sn_tab[found].name,
> > +                    MIN(sizeof(sn_tab[found].name), sizeof(inode.tag)),
> > +                    inode.tag);
> >              found++;
> >          }
> >      }
> 
>

Patch

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 0ed6b19..f2579da 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -852,14 +852,14 @@  static int parse_vdiname(BDRVSheepdogState *s, const char *filename,
         s->port = 0;
     }

-    strncpy(vdi, p, SD_MAX_VDI_LEN);
+    pstrcpy(vdi, SD_MAX_VDI_LEN, p);

     p = strchr(vdi, ':');
     if (p) {
         *p++ = '\0';
         *snapid = strtoul(p, NULL, 10);
         if (*snapid == 0) {
-            strncpy(tag, p, SD_MAX_VDI_TAG_LEN);
+            pstrcpy(tag, SD_MAX_VDI_TAG_LEN, p);
         }
     } else {
         *snapid = CURRENT_VDI_ID; /* search current vdi */
@@ -886,7 +886,10 @@  static int find_vdi_name(BDRVSheepdogState *s, char *filename, uint32_t snapid,
         return -1;
     }

-    memset(buf, 0, sizeof(buf));
+    /* This pair of strncpy calls ensures that the buffer is zero-filled,
+     * which is desirable since we'll soon be sending those bytes, and
+     * don't want the send_req to read uninitialized data.
+     */
     strncpy(buf, filename, SD_MAX_VDI_LEN);
     strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);

@@ -1129,7 +1132,7 @@  static int sd_open(BlockDriverState *bs, const char *filename, int flags)
     s->max_dirty_data_idx = 0;

     bs->total_sectors = s->inode.vdi_size / SECTOR_SIZE;
-    strncpy(s->name, vdi, sizeof(s->name));
+    pstrcpy(s->name, sizeof(s->name), vdi);
     qemu_co_mutex_init(&s->lock);
     g_free(buf);
     return 0;
@@ -1157,8 +1160,11 @@  static int do_sd_create(char *filename, int64_t vdi_size,
         return -EIO;
     }

+    /* FIXME: would it be better to fail (e.g., return -EIO) when filename
+     * does not fit in buf?  For now, just truncate and avoid buffer overrun.
+     */
     memset(buf, 0, sizeof(buf));
-    strncpy(buf, filename, SD_MAX_VDI_LEN);
+    pstrcpy(buf, sizeof(buf), filename);

     memset(&hdr, 0, sizeof(hdr));
     hdr.opcode = SD_OP_NEW_VDI;
@@ -1709,6 +1715,9 @@  static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)

     s->inode.vm_state_size = sn_info->vm_state_size;
     s->inode.vm_clock_nsec = sn_info->vm_clock_nsec;
+    /* It appears that inode.tag does not require a NUL terminator,
+     * which means this use of strncpy is ok.
+     */
     strncpy(s->inode.tag, sn_info->name, sizeof(s->inode.tag));
     /* we don't need to update entire object */
     datalen = SD_INODE_SIZE - sizeof(s->inode.data_vdi_id);
@@ -1771,13 +1780,13 @@  static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)

     memcpy(old_s, s, sizeof(BDRVSheepdogState));

-    memset(vdi, 0, sizeof(vdi));
-    strncpy(vdi, s->name, sizeof(vdi));
+    pstrcpy(vdi, sizeof(vdi), s->name);

-    memset(tag, 0, sizeof(tag));
     snapid = strtoul(snapshot_id, NULL, 10);
-    if (!snapid) {
-        strncpy(tag, s->name, sizeof(tag));
+    if (snapid) {
+        tag[0] = 0;
+    } else {
+        pstrcpy(tag, sizeof(tag), s->name);
     }

     ret = find_vdi_name(s, vdi, snapid, tag, &vid, 1);
@@ -1905,8 +1914,9 @@  static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)

             snprintf(sn_tab[found].id_str, sizeof(sn_tab[found].id_str), "%u",
                      inode.snap_id);
-            strncpy(sn_tab[found].name, inode.tag,
-                    MIN(sizeof(sn_tab[found].name), sizeof(inode.tag)));
+            pstrcpy(sn_tab[found].name,
+                    MIN(sizeof(sn_tab[found].name), sizeof(inode.tag)),
+                    inode.tag);
             found++;
         }
     }