Skip to contentSkip to conversation

fix an endianness bug in ext4_encrypted_follow_link()

PATCH Applied Al Viro 5 participants 7 messages Nov 26, 2015 – Nov 27, 2015 ext4 file system

lore
Share & tools
lore

Al Viro

author
#0 lore raw

PATCH 1/1 fix an endianness bug in ext4_encrypted_follow_link()

applying le32_to_cpu() to 16bit value is a bad idea...

Cc [email protected] # v4.1+
Signed-off-by Al Viro <[email protected]>

---

fs/ext4/symlink.c +1 -1
index abe2401..e8e7af6 100644
@@ -52,7 +52,7 @@ static const char *ext4_encrypted_follow_link(struct dentry *dentry, void **cook
 	/* Symlink is encrypted */
 	sd = (struct ext4_encrypted_symlink_data *)caddr;
 	cstr.name = sd->encrypted_path;
-	cstr.len  = le32_to_cpu(sd->len);
+	cstr.len  = le16_to_cpu(sd->len);
 	if ((cstr.len +
 	     sizeof(struct ext4_encrypted_symlink_data) - 1) >
 	    max_size) {
#1 lore raw

(cc'ing Julia Lawall)

Quoted 1 line · Al Viro

applying le32_to_cpu() to 16bit value is a bad idea...

Julia, perhaps you or your crew could produce a coccinelle test for this class of error?

Quoted 13 lines
fs/ext4/symlink.c +1 -1
index abe2401..e8e7af6 100644
@@ -52,7 +52,7 @@ static const char *ext4_encrypted_follow_link(struct dentry *dentry, void **cook
 	/* Symlink is encrypted */
 	sd = (struct ext4_encrypted_symlink_data *)caddr;
 	cstr.name = sd->encrypted_path;
-	cstr.len  = le32_to_cpu(sd->len);
+	cstr.len  = le16_to_cpu(sd->len);
 	if ((cstr.len +
 	     sizeof(struct ext4_encrypted_symlink_data) - 1) >
 	    max_size) {
author
#2 lore raw
Quoted 7 lines · Joe Perches

(cc'ing Julia Lawall)

Quoted 1 line · Al Viro

applying le32_to_cpu() to 16bit value is a bad idea...

Julia, perhaps you or your crew could produce a coccinelle test for this class of error?

What's wrong with something like make C=2 CF=-D__CHECK_ENDIAN__ fs/ext4/ ? Worked just fine, TYVM -
CHECK fs/ext4/symlink.c
fs/ext4/symlink.c:55:21: warning: cast to restricted __le32 fs/ext4/symlink.c:55:21: warning: cast from restricted __le16 points to exact location... Sure, you need to figure out _how_ it's broken (e.g. with something like

struct foo {
	__le32 a;
	__le16 b;
} *p;

le32_to_cpu(p->b) might have been misspelled le16_to_cpu(p->b) as well as le32_to_cpu(p->a)) - no way to tell one from another without actually reading and understanding the code in question. But that doesn't depend upon the tool used to locate the damn thing and sparse does locate them...

#3 lore raw
Quoted 16 lines · Al Viro
Quoted 7 lines · Joe Perches

(cc'ing Julia Lawall)

Quoted 1 line · Al Viro

applying le32_to_cpu() to 16bit value is a bad idea...

Julia, perhaps you or your crew could produce a coccinelle test for this class of error?

What's wrong with something like make C=2 CF=-D__CHECK_ENDIAN__ fs/ext4/ ? Worked just fine, TYVM -
CHECK fs/ext4/symlink.c
fs/ext4/symlink.c:55:21: warning: cast to restricted __le32 fs/ext4/symlink.c:55:21: warning: cast from restricted __le16 points to exact location... Sure, you need to figure out _how_ it's broken (e.g. with something like

Quoted 8 lines
struct foo {
	__le32 a;
	__le16 b;
} *p;

le32_to_cpu(p->b) might have been misspelled le16_to_cpu(p->b) as well as le32_to_cpu(p->a)) - no way to tell one from another without actually reading and understanding the code in question. But that doesn't depend upon the tool used to locate the damn thing and sparse does locate them...

As long as the code of interest is getting compiled in the current configuration, relying on the compiler fo this seems like a better choice. Coccinelle has no idea what types represent 16 values, and if one were to try to enumerate them one would surely miss something.

julia

#4 lore raw
Quoted 10 lines · Julia Lawall
Quoted 6 lines · Al Viro

On Thu, Nov 26, 2015 at 11:40:37AM -0800, Joe Perches wrote:
(cc'ing Julia Lawall)

Quoted 2 lines · Al Viro

applying le32_to_cpu() to 16bit value is a bad idea...

Julia, perhaps you or your crew could produce a coccinelle test for this class of error?

What's wrong with something like make C=2 CF=-D__CHECK_ENDIAN__ fs/ext4/ ? Worked just fine, TYVM -
sparse does locate them...

Nothing at all.

Quoted 2 lines

As long as the code of interest is getting compiled in the current configuration, relying on the compiler for this seems like a better choice.

Sparse isn't the compiler, but that would be fine by me as long as something can catch them.

The original commit (f348c252320b9) was from April. Isn't the kbuild robot using sparse and __CHECK_ENDIAN__?

build bot
#5 lore raw
Yes 0day did catch the sparse warning, however it seems the email somehow failed to get delivered. Here is the local record:

On Thu, Nov 26, 2015 at 02:47:56PM -0800, Joe Perches wrote:

> On Thu, 2015-11-26 at 22:28 +0100, Julia Lawall wrote:
> > On Thu, 26 Nov 2015, Al Viro wrote:
> > > On Thu, Nov 26, 2015 at 11:40:37AM -0800, Joe Perches wrote:
> > > (cc'ing Julia Lawall)
> > > > On Thu, 2015-11-26 at 15:27 +0000, Al Viro wrote:
> > > > applying le32_to_cpu() to 16bit value is a bad idea...
> > > Julia, perhaps you or your crew could produce a coccinelle test
> > > for this class of error?
> > What's wrong with something like make C=2 CF=-D__CHECK_ENDIAN__ fs/ext4/ ?
> > Worked just fine, TYVM -
> > sparse does locate them...
> 
> Nothing at all.
> 
> > As long as the code of interest is getting compiled in the current
> > configuration, relying on the compiler for this seems like a better choice.
> 
> Sparse isn't the compiler, but that would be fine by me
> as long as something can catch them.
> 
> The original commit (f348c252320b9) was from April.
> Isn't the kbuild robot using sparse and __CHECK_ENDIAN__?

Yes 0day did catch the sparse warning, however it seems the email somehow failed to get delivered. Here is the local record:

        Date: Mon, 13 Apr 2015 16:41:54 +0800
        From: kbuild test robot <[email protected]>
        To: Theodore Ts'o <[email protected]>
        Cc: [email protected], Uday Savagaonkar <[email protected]>
        Subject: [ext4:dev 32/33] fs/ext4/namei.c:3262:25: sparse: incorrect type in
         assignment (different base types)
        Message-ID: <201504131652.Ox8dW5C0%[email protected]>
        MIME-Version: 1.0
        Content-Type: text/plain; charset=us-ascii
        Content-Disposition: inline
        User-Agent: Mutt/1.5.23 (2014-03-12)

        tree:   git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
        head:   3a19824f63e0a0df99c0a133097eb87c0152545e
        commit: f1195c72c95115858123813e9a84badad37424c0 [32/33] ext4 crypto: Add symlink encryption
        reproduce:
          # apt-get install sparse
          git checkout f1195c72c95115858123813e9a84badad37424c0
          make ARCH=x86_64 allmodconfig
          make C=1 CF=-D__CHECK_ENDIAN__

        sparse warnings: (new ones prefixed by >>)

        >> fs/ext4/namei.c:3262:25: sparse: incorrect type in assignment (different base types)
           fs/ext4/namei.c:3262:25:    expected restricted __le16 [usertype] len
           fs/ext4/namei.c:3262:25:    got restricted __le32 [usertype] <noident>
        --
        >> fs/ext4/symlink.c:74:29: sparse: cast to restricted __le32
        >> fs/ext4/symlink.c:74:29: sparse: cast from restricted __le16

Thanks,
Fengguang