Re: [PATCH] fix an endianness bug in ext4_encrypted_follow_link()
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 +, 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 To: Theodore Ts'o Cc: kbuild-...@01.org, Uday Savagaonkar Subject: [ext4:dev 32/33] fs/ext4/namei.c:3262:25: sparse: incorrect type in assignment (different base types) Message-ID: <201504131652.ox8dw5c0%fengguang...@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] -- >> 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix an endianness bug in ext4_encrypted_follow_link()
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 +, 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix an endianness bug in ext4_encrypted_follow_link()
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 +, 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix an endianness bug in ext4_encrypted_follow_link()
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 +, 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix an endianness bug in ext4_encrypted_follow_link()
On Thu, Nov 26, 2015 at 03:27:28PM +, Al Viro wrote: > applying le32_to_cpu() to 16bit value is a bad idea... > > Cc: sta...@vger.kernel.org # v4.1+ > Signed-off-by: Al Viro Thanks, applied. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] fix an endianness bug in ext4_encrypted_follow_link()
(cc'ing Julia Lawall)
On Thu, 2015-11-26 at 15:27 +, 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
