Bug 719135

Summary: GFS2: locktable and label setting issues
Product: Red Hat Enterprise Linux 6 Reporter: Nate Straz <nstraz>
Component: clusterAssignee: Andrew Price <anprice>
Status: CLOSED ERRATA QA Contact: Cluster QE <mspqa-list>
Severity: high Docs Contact:
Priority: high    
Version: 6.2CC: ccaulfie, cluster-maint, lhh, rpeterso, swhiteho, teigland
Target Milestone: rc   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: cluster-3.0.12.1-7.el6 Doc Type: Bug Fix
Doc Text:
Prior to this fix, boundaries for the locktable and label fields in the GFS2 superblock were not properly checked by the tunegfs2 tool. That allowed you to cause "gfs2_tool sb" to abort abnormally (buffer overflow) or to print invalid characters by using tunegfs2 to change the locktable or label to a minimum or maximum length (63 characters). Code was added to tunefs2 to check the correct boundaries of the locktable and label fields. As a result, tunegfs2 will no longer create invalid locktables or labels, and therefore, gfs2_tool will print the superblock values properly.
 Story Points: ---
Clone Of: Environment:
Last Closed: 2011-12-06 14:52:29 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 704178    
Attachments:
Description Flags
Upstream patch to fix bug none

Description Nate Straz 2011-07-05 20:32:48 UTC 
Description of problem:

This is a collection of several issues found while testing the new tunegfs2 command with respect to lock table and label setting functions, -L and -o locktable=X.

1. If no lock table is specified, a label can not be set

# mkfs -t gfs2 -p lock_nolock -j 1  /dev/sdb1
# tunegfs2 -L A  /dev/sdb1
Label too long

2. tunegfs2 will trigger a buffer overflow if length(locktable + label) = 63 when setting the locktable name

# mkfs -t gfs2 -p lock_nolock -j 1 -t foo:longername -O /dev/sdb1
Device:                    /dev/sdb1
Blocksize:                 4096
Device Size                135.67 GB (35563885 blocks)
Filesystem Size:           135.67 GB (35563883 blocks)
Journals:                  1
Resource Groups:           543
Locking Protocol:          "lock_nolock"
Lock Table:                "foo:longername"
UUID:                      7a16773f-fbae-8863-0b92-6d83ddfe5426

# tunegfs2 -L 1234567890123456789012345678901234567890123456789012345 /dev/sdb1
# tunegfs2 -o locktable=buzzezz /dev/sdb1
# gfs2_tool sb /dev/sdb1 all
  mh_magic = 0x01161970
  mh_type = 1
  mh_format = 100
  sb_fs_format = 1801
  sb_multihost_format = 1900
  sb_bsize = 4096
  sb_bsize_shift = 12
  no_formal_ino = 2
  no_addr = 23
  no_formal_ino = 1
  no_addr = 22
  sb_lockproto = lock_nolock
  sb_locktable = buzzezz:1234567890123456789012345678901234567890123456789012345
  uuid = 7a16773f-fbae-8863-0b92-6d83ddfe5426
# wc -c
buzzezz:1234567890123456789012345678901234567890123456789012345
63
# tunegfs2 -l /dev/sdb1
tunegfs2 (Jun 23 2011 05:03:50)
Filesystem volume name: 1234567890123456789012345678901234567890123456789012345
Filesystem UUID: 7a16773f-fbae-8863-0b92-6d83ddfe5426
Filesystem magic number: 0x1161970
Block size: 4096
Block shift: 12
Root inode: 22
Master inode: 23
Lock Protocol: lock_nolock
Lock table: buzzezz
# tunegfs2 -o locktable=buzzezzz /dev/sdb1
*** buffer overflow detected ***: tunegfs2 terminated
======= Backtrace: =========
/lib64/libc.so.6(__fortify_fail+0x37)[0x31c94ff527]
/lib64/libc.so.6[0x31c94fd410]
/lib64/libc.so.6[0x31c94fc869]
/lib64/libc.so.6(_IO_default_xsputn+0xc9)[0x31c9473a59]
/lib64/libc.so.6(_IO_vfprintf+0x3826)[0x31c94476d6]
/lib64/libc.so.6(__vsprintf_chk+0x9d)[0x31c94fc90d]
/lib64/libc.so.6(__sprintf_chk+0x7f)[0x31c94fc84f]
tunegfs2[0x40157a]
tunegfs2[0x400f0a]
/lib64/libc.so.6(__libc_start_main+0xfd)[0x31c941ecdd]
tunegfs2[0x400ac9]

3. Setting the label name such that the locktable and label hit the 63 character limit causes `gfs2_tool sb $dev table` to print beyond the end of the lock table name

# tunegfs2 -L 12345678901234567890123456789012345678901234567890123456 /dev/sdb1
# tunegfs2 -l /dev/sdb1
tunegfs2 (Jun 23 2011 05:03:50)
Filesystem volume name: 12345678901234567890123456789012345678901234567890123456
Filesystem UUID: 7a16773f-fbae-8863-0b92-6d83ddfe5426
Filesystem magic number: 0x1161970
Block size: 4096
Block shift: 12
Root inode: 22
Master inode: 23
Lock Protocol: lock_nolock
Lock table: buzzezz
# gfs2_tool sb /dev/sdb1 table
current lock table name = "buzzezz:12345678901234567890123456789012345678901234567890123456°"
# gfs2_tool sb /dev/sdb1 table
current lock table name = "buzzezz:12345678901234567890123456789012345678901234567890123456p"


Version-Release number of selected component (if applicable):
gfs2-utils-3.0.12.1-5.el6.x86_64


How reproducible:
Easily

Steps to Reproduce:
See steps in description
  
Actual results:


Expected results:


Additional info:
Comment 2 Steve Whitehouse 2011-07-22 10:10:49 UTC 
Created attachment 514649 [details]
Upstream patch to fix bug
Comment 3 Andrew Price 2011-07-25 18:17:08 UTC 
The upstream patch applies cleanly to the RHEL6 branch of cluster.git so I've added my Reviewed-by line and pushed the commit. Setting to POST.
Comment 4 Andrew Price 2011-07-26 14:22:26 UTC 
https://brewweb.devel.redhat.com/buildinfo?buildID=173797
Comment 6 Andrew Price 2011-07-27 08:52:54 UTC 
Testing gfs2-utils-3.0.12.1-7.el6.x86_64.rpm with original steps to reproduce:

# mkfs -t gfs2 -O -p lock_nolock -j 1 sparse-file 
# tunegfs2 -L A sparse-file 
# tunegfs2 -l sparse-file 
tunegfs2 (Jul 26 2011 10:11:28)
Filesystem volume name: A
Filesystem UUID: 8406fc34-b5dc-10da-ae8a-a939b18015bc
Filesystem magic number: 0x1161970
Block size: 4096
Block shift: 12
Root inode: 22
Master inode: 23
Lock Protocol: lock_nolock
Lock table: A
# mkfs -t gfs2 -p lock_nolock -j 1 -t foo:longername -O sparse-file 
Device:                    sparse-file
Blocksize:                 4096
Device Size                10.00 GB (2621440 blocks)
Filesystem Size:           10.00 GB (2621438 blocks)
Journals:                  1
Resource Groups:           40
Locking Protocol:          "lock_nolock"
Lock Table:                "foo:longername"
UUID:                      1551398e-1876-3e8f-8687-2897e9d39e0f

# tunegfs2 -o locktable=buzzezz:1234567890123456789012345678901234567890123456789012345 sparse-file 
# tunegfs2 -l sparse-file 
tunegfs2 (Jul 26 2011 10:11:28)
Filesystem volume name: buzzezz:1234567890123456789012345678901234567890123456789012345
Filesystem UUID: 1551398e-1876-3e8f-8687-2897e9d39e0f
Filesystem magic number: 0x1161970
Block size: 4096
Block shift: 12
Root inode: 22
Master inode: 23
Lock Protocol: lock_nolock
Lock table: buzzezz:1234567890123456789012345678901234567890123456789012345
# tunegfs2 -o locktable=buzzezzz:1234567890123456789012345678901234567890123456789012345 sparse-file 
Lock table name too long
# gfs2_tool sb sparse-file table
current lock table name = "buzzezz:1234567890123456789012345678901234567890123456789012345"
Comment 7 Nate Straz 2011-07-29 18:59:25 UTC 
Looks like the behavior changed slightly.  Now it looks like Filesystem volume name and Lock Table are the same field, which agrees now with the man page.

Before the Lock Table was two parts, <lock table>:<volume name>.  This would be set by `tunegfs2 -o locktable=<locktable>` and `tunegfs2 -L <volume name>.`

I just want to verify that this change in behavior was intended.

Also, since they are now the same field, do we really need -o locktable?
Should lockproto be moved to a option like -p?
Comment 8 Andrew Price 2011-08-02 09:52:49 UTC 
As this is technically Steve's patch I think he can answer comment #7 more authoritatively than I can.
Comment 9 Steve Whitehouse 2011-08-02 10:03:10 UTC 
Yes, it was intended. Not only does it now agree with the man page, but also I realised that it was going to cause all kinds of issues if we tried to separate the two.

The arguments were designed to match those for tune2fs so they are just synonyms and I don't think there is any problem in leaving them "as is". We could add a -p option since tune2fs doesn't have that already, but I'm not sure that we really need to add yet another method to the tool. I don't mind doing that in the future, but its not really relevant to this bz I think.

The main aim here was to ensure that the lengths of the input were being properly checked, and to fix the slightly odd behaviour of the -L flag.
Comment 10 Robert Peterson 2011-10-27 15:33:21 UTC 
    Technical note added. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    New Contents:
Prior to this fix, boundaries for the locktable and label fields in the GFS2 superblock were not properly checked by the tunegfs2 tool. That allowed you to cause "gfs2_tool sb" to abort abnormally (buffer overflow) or to print invalid characters by using tunegfs2 to change the locktable or label to a minimum or maximum length (63 characters). Code was added to tunefs2 to check the correct boundaries of the locktable and label fields. As a result, tunegfs2 will no longer create invalid locktables or labels, and therefore, gfs2_tool will print the superblock values properly.
Comment 11 errata-xmlrpc 2011-12-06 14:52:29 UTC 
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2011-1516.html