From: "Dr. David Alan Gilbert" <[email protected]>
JFS has in jfs_incore.h:
/* _inline may overflow into _inline_ea when needed */
/* _inline_ea may overlay the last part of
* file._xtroot if maxentry = XTROOTINITSLOT
*/
union {
struct {
/* 128: inline symlink */
unchar _inline[128];
/* 128: inline extended attr */
unchar _inline_ea[128];
};
unchar _inline_all[256];
and currently the symlink code copies into _inline;
if this is larger than 128 bytes it triggers a fortify warning of the
form:
memcpy: detected field-spanning write (size 132) of single field
"ip->i_link" at fs/jfs/namei.c:950 (size 18446744073709551615)
when it's actually OK.
Copy it into _inline_all instead.
Reported-by: [email protected]
Signed-off-by: Dr. David Alan Gilbert <[email protected]>
---
fs/jfs/namei.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/jfs/namei.c b/fs/jfs/namei.c
index 9db4f5789c0ec..4fbbf88435e69 100644
--- a/fs/jfs/namei.c
+++ b/fs/jfs/namei.c
@@ -946,7 +946,7 @@ static int jfs_symlink(struct user_namespace *mnt_userns, struct inode *dip,
if (ssize <= IDATASIZE) {
ip->i_op = &jfs_fast_symlink_inode_operations;
- ip->i_link = JFS_IP(ip)->i_inline;
+ ip->i_link = JFS_IP(ip)->i_inline_all;
memcpy(ip->i_link, name, ssize);
ip->i_size = ssize - 1;
--
2.37.3
On Sat, Oct 22, 2022 at 09:39:14PM +0100, [email protected] wrote:
> From: "Dr. David Alan Gilbert" <[email protected]>
>
> JFS has in jfs_incore.h:
>
> /* _inline may overflow into _inline_ea when needed */
> /* _inline_ea may overlay the last part of
> * file._xtroot if maxentry = XTROOTINITSLOT
> */
> union {
> struct {
> /* 128: inline symlink */
> unchar _inline[128];
> /* 128: inline extended attr */
> unchar _inline_ea[128];
> };
> unchar _inline_all[256];
>
> and currently the symlink code copies into _inline;
> if this is larger than 128 bytes it triggers a fortify warning of the
> form:
>
> memcpy: detected field-spanning write (size 132) of single field
> "ip->i_link" at fs/jfs/namei.c:950 (size 18446744073709551615)
Which compiler are you using for this build? This size report (SIZE_MAX)
should be impossible to reach. But also, the size is just wrong --
i_inline is 128 bytes, not SIZE_MAX. So, the detection is working
(132 > 128), but the report is broken, and I can't see how...
>
> when it's actually OK.
>
> Copy it into _inline_all instead.
>
> Reported-by: [email protected]
> Signed-off-by: Dr. David Alan Gilbert <[email protected]>
> ---
> fs/jfs/namei.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/jfs/namei.c b/fs/jfs/namei.c
> index 9db4f5789c0ec..4fbbf88435e69 100644
> --- a/fs/jfs/namei.c
> +++ b/fs/jfs/namei.c
> @@ -946,7 +946,7 @@ static int jfs_symlink(struct user_namespace *mnt_userns, struct inode *dip,
> if (ssize <= IDATASIZE) {
> ip->i_op = &jfs_fast_symlink_inode_operations;
>
> - ip->i_link = JFS_IP(ip)->i_inline;
> + ip->i_link = JFS_IP(ip)->i_inline_all;
> memcpy(ip->i_link, name, ssize);
> ip->i_size = ssize - 1;
>
Regardless, the fix looks correct to me!
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
* Kees Cook ([email protected]) wrote:
> On Sat, Oct 22, 2022 at 09:39:14PM +0100, [email protected] wrote:
> > From: "Dr. David Alan Gilbert" <[email protected]>
> >
> > JFS has in jfs_incore.h:
> >
> > /* _inline may overflow into _inline_ea when needed */
> > /* _inline_ea may overlay the last part of
> > * file._xtroot if maxentry = XTROOTINITSLOT
> > */
> > union {
> > struct {
> > /* 128: inline symlink */
> > unchar _inline[128];
> > /* 128: inline extended attr */
> > unchar _inline_ea[128];
> > };
> > unchar _inline_all[256];
> >
> > and currently the symlink code copies into _inline;
> > if this is larger than 128 bytes it triggers a fortify warning of the
> > form:
> >
> > memcpy: detected field-spanning write (size 132) of single field
> > "ip->i_link" at fs/jfs/namei.c:950 (size 18446744073709551615)
>
> Which compiler are you using for this build?
I think that report was the same on gcc on Fedora 37 and whatever
syzkaller was running.
> This size report (SIZE_MAX)
> should be impossible to reach. But also, the size is just wrong --
> i_inline is 128 bytes, not SIZE_MAX. So, the detection is working
> (132 > 128), but the report is broken, and I can't see how...
Yeh, and led me down a blind alley for a while thinking something had
really managed to screwup the strlen somehow.
>
> >
> > when it's actually OK.
> >
> > Copy it into _inline_all instead.
> >
> > Reported-by: [email protected]
> > Signed-off-by: Dr. David Alan Gilbert <[email protected]>
> > ---
> > fs/jfs/namei.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/jfs/namei.c b/fs/jfs/namei.c
> > index 9db4f5789c0ec..4fbbf88435e69 100644
> > --- a/fs/jfs/namei.c
> > +++ b/fs/jfs/namei.c
> > @@ -946,7 +946,7 @@ static int jfs_symlink(struct user_namespace *mnt_userns, struct inode *dip,
> > if (ssize <= IDATASIZE) {
> > ip->i_op = &jfs_fast_symlink_inode_operations;
> >
> > - ip->i_link = JFS_IP(ip)->i_inline;
> > + ip->i_link = JFS_IP(ip)->i_inline_all;
> > memcpy(ip->i_link, name, ssize);
> > ip->i_size = ssize - 1;
> >
>
> Regardless, the fix looks correct to me!
>
> Reviewed-by: Kees Cook <[email protected]>
Thanks!
Dave
> --
> Kees Cook
--
-----Open up your eyes, open up your mind, open up your code -------
/ Dr. David Alan Gilbert | Running GNU/Linux | Happy \
\ dave @ treblig.org | | In Hex /
\ _________________________|_____ http://www.treblig.org |_______/
Applied.
Thanks,
Shaggy
On 10/24/22 1:49PM, Dr. David Alan Gilbert wrote:
> * Kees Cook ([email protected]) wrote:
>> On Sat, Oct 22, 2022 at 09:39:14PM +0100, [email protected] wrote:
>>> From: "Dr. David Alan Gilbert" <[email protected]>
>>>
>>> JFS has in jfs_incore.h:
>>>
>>> /* _inline may overflow into _inline_ea when needed */
>>> /* _inline_ea may overlay the last part of
>>> * file._xtroot if maxentry = XTROOTINITSLOT
>>> */
>>> union {
>>> struct {
>>> /* 128: inline symlink */
>>> unchar _inline[128];
>>> /* 128: inline extended attr */
>>> unchar _inline_ea[128];
>>> };
>>> unchar _inline_all[256];
>>>
>>> and currently the symlink code copies into _inline;
>>> if this is larger than 128 bytes it triggers a fortify warning of the
>>> form:
>>>
>>> memcpy: detected field-spanning write (size 132) of single field
>>> "ip->i_link" at fs/jfs/namei.c:950 (size 18446744073709551615)
>>
>> Which compiler are you using for this build?
>
> I think that report was the same on gcc on Fedora 37 and whatever
> syzkaller was running.
>
>> This size report (SIZE_MAX)
>> should be impossible to reach. But also, the size is just wrong --
>> i_inline is 128 bytes, not SIZE_MAX. So, the detection is working
>> (132 > 128), but the report is broken, and I can't see how...
>
> Yeh, and led me down a blind alley for a while thinking something had
> really managed to screwup the strlen somehow.
>
>>
>>>
>>> when it's actually OK.
>>>
>>> Copy it into _inline_all instead.
>>>
>>> Reported-by: [email protected]
>>> Signed-off-by: Dr. David Alan Gilbert <[email protected]>
>>> ---
>>> fs/jfs/namei.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/jfs/namei.c b/fs/jfs/namei.c
>>> index 9db4f5789c0ec..4fbbf88435e69 100644
>>> --- a/fs/jfs/namei.c
>>> +++ b/fs/jfs/namei.c
>>> @@ -946,7 +946,7 @@ static int jfs_symlink(struct user_namespace *mnt_userns, struct inode *dip,
>>> if (ssize <= IDATASIZE) {
>>> ip->i_op = &jfs_fast_symlink_inode_operations;
>>>
>>> - ip->i_link = JFS_IP(ip)->i_inline;
>>> + ip->i_link = JFS_IP(ip)->i_inline_all;
>>> memcpy(ip->i_link, name, ssize);
>>> ip->i_size = ssize - 1;
>>>
>>
>> Regardless, the fix looks correct to me!
>>
>> Reviewed-by: Kees Cook <[email protected]>
>
> Thanks!
>
> Dave
>
>> --
>> Kees Cook
On Mon, Oct 24, 2022 at 07:49:17PM +0100, Dr. David Alan Gilbert wrote:
> * Kees Cook ([email protected]) wrote:
> > On Sat, Oct 22, 2022 at 09:39:14PM +0100, [email protected] wrote:
> > > From: "Dr. David Alan Gilbert" <[email protected]>
> > >
> > > JFS has in jfs_incore.h:
> > >
> > > /* _inline may overflow into _inline_ea when needed */
> > > /* _inline_ea may overlay the last part of
> > > * file._xtroot if maxentry = XTROOTINITSLOT
> > > */
> > > union {
> > > struct {
> > > /* 128: inline symlink */
> > > unchar _inline[128];
> > > /* 128: inline extended attr */
> > > unchar _inline_ea[128];
> > > };
> > > unchar _inline_all[256];
> > >
> > > and currently the symlink code copies into _inline;
> > > if this is larger than 128 bytes it triggers a fortify warning of the
> > > form:
> > >
> > > memcpy: detected field-spanning write (size 132) of single field
> > > "ip->i_link" at fs/jfs/namei.c:950 (size 18446744073709551615)
> >
> > Which compiler are you using for this build?
>
> I think that report was the same on gcc on Fedora 37 and whatever
> syzkaller was running.
>
> > This size report (SIZE_MAX)
> > should be impossible to reach. But also, the size is just wrong --
> > i_inline is 128 bytes, not SIZE_MAX. So, the detection is working
> > (132 > 128), but the report is broken, and I can't see how...
>
> Yeh, and led me down a blind alley for a while thinking something had
> really managed to screwup the strlen somehow.
This looks like a GCC bug (going at least back to GCC 10.2)[1], but some
extra care around the macro appears to make it go away, so the reporting
variable doesn't get confused/re-evaluated:
diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
index 09a032f6ce6b..9e2d96993c30 100644
--- a/include/linux/fortify-string.h
+++ b/include/linux/fortify-string.h
@@ -550,13 +550,18 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
#define __fortify_memcpy_chk(p, q, size, p_size, q_size, \
p_size_field, q_size_field, op) ({ \
- size_t __fortify_size = (size_t)(size); \
- WARN_ONCE(fortify_memcpy_chk(__fortify_size, p_size, q_size, \
- p_size_field, q_size_field, #op), \
+ const size_t __fortify_size = (size_t)(size); \
+ const size_t __p_size = (p_size); \
+ const size_t __q_size = (q_size); \
+ const size_t __p_size_field = (p_size_field); \
+ const size_t __q_size_field = (q_size_field); \
+ WARN_ONCE(fortify_memcpy_chk(__fortify_size, __p_size, \
+ __q_size, __p_size_field, \
+ __q_size_field, #op), \
#op ": detected field-spanning write (size %zu) of single %s (size %zu)\n", \
__fortify_size, \
"field \"" #p "\" at " __FILE__ ":" __stringify(__LINE__), \
- p_size_field); \
+ __p_size_field); \
__underlying_##op(p, q, __fortify_size); \
})
[1] https://syzkaller.appspot.com/bug?id=23d613df5259b977dac1696bec77f61a85890e3d
--
Kees Cook
* Kees Cook ([email protected]) wrote:
> On Mon, Oct 24, 2022 at 07:49:17PM +0100, Dr. David Alan Gilbert wrote:
> > * Kees Cook ([email protected]) wrote:
> > > On Sat, Oct 22, 2022 at 09:39:14PM +0100, [email protected] wrote:
> > > > From: "Dr. David Alan Gilbert" <[email protected]>
> > > >
> > > > JFS has in jfs_incore.h:
> > > >
> > > > /* _inline may overflow into _inline_ea when needed */
> > > > /* _inline_ea may overlay the last part of
> > > > * file._xtroot if maxentry = XTROOTINITSLOT
> > > > */
> > > > union {
> > > > struct {
> > > > /* 128: inline symlink */
> > > > unchar _inline[128];
> > > > /* 128: inline extended attr */
> > > > unchar _inline_ea[128];
> > > > };
> > > > unchar _inline_all[256];
> > > >
> > > > and currently the symlink code copies into _inline;
> > > > if this is larger than 128 bytes it triggers a fortify warning of the
> > > > form:
> > > >
> > > > memcpy: detected field-spanning write (size 132) of single field
> > > > "ip->i_link" at fs/jfs/namei.c:950 (size 18446744073709551615)
> > >
> > > Which compiler are you using for this build?
> >
> > I think that report was the same on gcc on Fedora 37 and whatever
> > syzkaller was running.
> >
> > > This size report (SIZE_MAX)
> > > should be impossible to reach. But also, the size is just wrong --
> > > i_inline is 128 bytes, not SIZE_MAX. So, the detection is working
> > > (132 > 128), but the report is broken, and I can't see how...
> >
> > Yeh, and led me down a blind alley for a while thinking something had
> > really managed to screwup the strlen somehow.
>
> This looks like a GCC bug (going at least back to GCC 10.2)[1], but some
> extra care around the macro appears to make it go away, so the reporting
> variable doesn't get confused/re-evaluated:
Thanks for chasing that; are you also going to file a gcc bug?
Dave
> diff --git a/include/linux/fortify-string.h b/include/linux/fortify-string.h
> index 09a032f6ce6b..9e2d96993c30 100644
> --- a/include/linux/fortify-string.h
> +++ b/include/linux/fortify-string.h
> @@ -550,13 +550,18 @@ __FORTIFY_INLINE bool fortify_memcpy_chk(__kernel_size_t size,
>
> #define __fortify_memcpy_chk(p, q, size, p_size, q_size, \
> p_size_field, q_size_field, op) ({ \
> - size_t __fortify_size = (size_t)(size); \
> - WARN_ONCE(fortify_memcpy_chk(__fortify_size, p_size, q_size, \
> - p_size_field, q_size_field, #op), \
> + const size_t __fortify_size = (size_t)(size); \
> + const size_t __p_size = (p_size); \
> + const size_t __q_size = (q_size); \
> + const size_t __p_size_field = (p_size_field); \
> + const size_t __q_size_field = (q_size_field); \
> + WARN_ONCE(fortify_memcpy_chk(__fortify_size, __p_size, \
> + __q_size, __p_size_field, \
> + __q_size_field, #op), \
> #op ": detected field-spanning write (size %zu) of single %s (size %zu)\n", \
> __fortify_size, \
> "field \"" #p "\" at " __FILE__ ":" __stringify(__LINE__), \
> - p_size_field); \
> + __p_size_field); \
> __underlying_##op(p, q, __fortify_size); \
> })
>
>
>
> [1] https://syzkaller.appspot.com/bug?id=23d613df5259b977dac1696bec77f61a85890e3d
>
> --
> Kees Cook
--
-----Open up your eyes, open up your mind, open up your code -------
/ Dr. David Alan Gilbert | Running GNU/Linux | Happy \
\ dave @ treblig.org | | In Hex /
\ _________________________|_____ http://www.treblig.org |_______/
On Sat, Oct 29, 2022 at 01:48:05PM +0100, Dr. David Alan Gilbert wrote:
> * Kees Cook ([email protected]) wrote:
> > This looks like a GCC bug (going at least back to GCC 10.2)[1], but some
> > extra care around the macro appears to make it go away, so the reporting
> > variable doesn't get confused/re-evaluated:
>
> Thanks for chasing that; are you also going to file a gcc bug?
No, I haven't had the time to minimize a PoC, so I'm not sure it would
make a very good bug report.
--
Kees Cook