diff mbox

fix an endianness bug in ext4_encrypted_follow_link()

Message ID 20151126152728.GT22011@ZenIV.linux.org.uk
State Accepted, archived
Headers show

Commit Message

Al Viro Nov. 26, 2015, 3:27 p.m. UTC 
applying le32_to_cpu() to 16bit value is a bad idea...
    
Cc: stable@vger.kernel.org # v4.1+
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Joe Perches Nov. 26, 2015, 7:40 p.m. UTC | #1 
(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?

> diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
> index abe2401..e8e7af6 100644
> --- a/fs/ext4/symlink.c
> +++ b/fs/ext4/symlink.c
> @@ -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) {

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o Nov. 26, 2015, 8:22 p.m. UTC | #2 
On Thu, Nov 26, 2015 at 03:27:28PM +0000, Al Viro wrote:
> applying le32_to_cpu() to 16bit value is a bad idea...
>     
> Cc: stable@vger.kernel.org # v4.1+
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Thanks, applied.

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro Nov. 26, 2015, 9:02 p.m. UTC | #3 
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 -
  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...
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julia Lawall Nov. 26, 2015, 9:28 p.m. UTC | #4 
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 -
>   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...

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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches Nov. 26, 2015, 10:47 p.m. UTC | #5 
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__?

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
kernel test robot Nov. 27, 2015, 1:31 a.m. UTC | #6 
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 <fengguang.wu@intel.com>
        To: Theodore Ts'o <tytso@mit.edu>
        Cc: kbuild-all@01.org, Uday Savagaonkar <savagaon@google.com>
        Subject: [ext4:dev 32/33] fs/ext4/namei.c:3262:25: sparse: incorrect type in
         assignment (different base types)
        Message-ID: <201504131652.Ox8dW5C0%fengguang.wu@intel.com>
        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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
index abe2401..e8e7af6 100644
--- a/fs/ext4/symlink.c
+++ b/fs/ext4/symlink.c
@@ -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) {