* [PATCH] udf: fix uninit-value use in udf_get_fileshortad
@ 2024-09-19 19:52 Gianfranco Trad
0 siblings, 0 replies; 3+ messages in thread
From: Gianfranco Trad @ 2024-09-19 19:52 UTC (permalink / raw)
To: jack; +Cc: linux-kernel, skhan, Gianfranco Trad, syzbot+8901c4560b7ab5c2f9df
Syzbot reported a udf uninit-value use in udf_get_fileshortad[1].
The uninit-value use occurs when udf_get_fileshortad is called
with alen having a negative value and epos->offset being equal
to epos->bh->b_size. This check was inserted to mitigate
the uninit-value use. After applying the patch reproducer did
not trigget any issue[2].
[1] https://syzkaller.appspot.com/bug?extid=8901c4560b7ab5c2f9df
[2] https://syzkaller.appspot.com/x/log.txt?x=133ab69f980000
Reported-by: syzbot+8901c4560b7ab5c2f9df@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=8901c4560b7ab5c2f9df
Tested-by: syzbot+8901c4560b7ab5c2f9df@syzkaller.appspotmail.com
Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com>
---
fs/udf/inode.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 4726a4d014b6..d21665164779 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -2222,6 +2222,8 @@ int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
switch (iinfo->i_alloc_type) {
case ICBTAG_FLAG_AD_SHORT:
+ if (unlikely(alen < 0 && epos->offset == epos->bh->b_size))
+ return -1;
sad = udf_get_fileshortad(ptr, alen, &epos->offset, inc);
if (!sad)
return -1;
--
2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread[parent not found: <202409230809.5ozgjLm9-lkp@intel.com>]
* Re: [PATCH] udf: fix uninit-value use in udf_get_fileshortad
[not found] <202409230809.5ozgjLm9-lkp@intel.com>
@ 2024-09-23 9:23 ` Gianfranco Trad
2024-09-23 14:22 ` Jan Kara
0 siblings, 1 reply; 3+ messages in thread
From: Gianfranco Trad @ 2024-09-23 9:23 UTC (permalink / raw)
To: kernel test robot, oe-kbuild
Cc: Dan Carpenter, jack, linux-kernel, syzbot+8901c4560b7ab5c2f9df
Hello,
On 23/09/24 02:26, kernel test robot wrote:
> BCC: lkp@intel.com
> CC: oe-kbuild-all@lists.linux.dev
> In-Reply-To: <20240919195227.412583-1-gianf.trad@gmail.com>
> References: <20240919195227.412583-1-gianf.trad@gmail.com>
> TO: Gianfranco Trad <gianf.trad@gmail.com>
> TO: jack@suse.com
> CC: linux-kernel@vger.kernel.org
> CC: skhan@linuxfoundation.org
> CC: Gianfranco Trad <gianf.trad@gmail.com>
> CC: syzbot+8901c4560b7ab5c2f9df@syzkaller.appspotmail.com
>
> Hi Gianfranco,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on brauner-vfs/vfs.all]
> [also build test WARNING on linus/master v6.11 next-20240920]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Gianfranco-Trad/udf-fix-uninit-value-use-in-udf_get_fileshortad/20240920-035519
> base: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
> patch link: https://lore.kernel.org/r/20240919195227.412583-1-gianf.trad%40gmail.com
> patch subject: [PATCH] udf: fix uninit-value use in udf_get_fileshortad
> :::::: branch date: 3 days ago
> :::::: commit date: 3 days ago
> config: mips-randconfig-r073-20240922 (https://download.01.org/0day-ci/archive/20240923/202409230809.5ozgjLm9-lkp@intel.com/config)
> compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> | Closes: https://lore.kernel.org/r/202409230809.5ozgjLm9-lkp@intel.com/
>
> smatch warnings:
> fs/udf/inode.c:2223 udf_current_aext() error: we previously assumed 'epos->bh' could be null (see line 2204)
Given this error my initial guess is to update the patch to check the
assumption of epos->bh not null.
> vim +2223 fs/udf/inode.c
>
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2193
> ff116fc8d1d439 Jan Kara 2007-05-08 2194 int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
> 5ca4e4be841e38 Pekka Enberg 2008-10-15 2195 struct kernel_lb_addr *eloc, uint32_t *elen, int inc)
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2196 {
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2197 int alen;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2198 int8_t etype;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2199 uint8_t *ptr;
> 5ca4e4be841e38 Pekka Enberg 2008-10-15 2200 struct short_ad *sad;
> 5ca4e4be841e38 Pekka Enberg 2008-10-15 2201 struct long_ad *lad;
> 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2202 struct udf_inode_info *iinfo = UDF_I(inode);
> 28de7948a89676 Cyrill Gorcunov 2007-07-21 2203
> cb00ea3528eb3c Cyrill Gorcunov 2007-07-19 @2204 if (!epos->bh) {
> ff116fc8d1d439 Jan Kara 2007-05-08 2205 if (!epos->offset)
> ff116fc8d1d439 Jan Kara 2007-05-08 2206 epos->offset = udf_file_entry_alloc_offset(inode);
> 382a2287bf9cd2 Jan Kara 2020-09-25 2207 ptr = iinfo->i_data + epos->offset -
> 4b11111aba6c80 Marcin Slusarz 2008-02-08 2208 udf_file_entry_alloc_offset(inode) +
> 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2209 iinfo->i_lenEAttr;
> 4b11111aba6c80 Marcin Slusarz 2008-02-08 2210 alen = udf_file_entry_alloc_offset(inode) +
> 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2211 iinfo->i_lenAlloc;
> cb00ea3528eb3c Cyrill Gorcunov 2007-07-19 2212 } else {
> ff116fc8d1d439 Jan Kara 2007-05-08 2213 if (!epos->offset)
> ff116fc8d1d439 Jan Kara 2007-05-08 2214 epos->offset = sizeof(struct allocExtDesc);
> ff116fc8d1d439 Jan Kara 2007-05-08 2215 ptr = epos->bh->b_data + epos->offset;
> 28de7948a89676 Cyrill Gorcunov 2007-07-21 2216 alen = sizeof(struct allocExtDesc) +
> 4b11111aba6c80 Marcin Slusarz 2008-02-08 2217 le32_to_cpu(((struct allocExtDesc *)epos->bh->b_data)->
> 4b11111aba6c80 Marcin Slusarz 2008-02-08 2218 lengthAllocDescs);
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2219 }
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2220
> 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2221 switch (iinfo->i_alloc_type) {
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2222 case ICBTAG_FLAG_AD_SHORT:
> 2aa242080b8dda Gianfranco Trad 2024-09-19 @2223 if (unlikely(alen < 0 && epos->offset == epos->bh->b_size))
> 2aa242080b8dda Gianfranco Trad 2024-09-19 2224 return -1;
The check would be inserted up here in the if clause. But if the patch
doesn't look good, I can redesign it in a better way. If so, I'm more
than glad to follow your advice.
> 4b11111aba6c80 Marcin Slusarz 2008-02-08 2225 sad = udf_get_fileshortad(ptr, alen, &epos->offset, inc);
> 4b11111aba6c80 Marcin Slusarz 2008-02-08 2226 if (!sad)
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2227 return -1;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2228 etype = le32_to_cpu(sad->extLength) >> 30;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2229 eloc->logicalBlockNum = le32_to_cpu(sad->extPosition);
> 4b11111aba6c80 Marcin Slusarz 2008-02-08 2230 eloc->partitionReferenceNum =
> 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2231 iinfo->i_location.partitionReferenceNum;
> 28de7948a89676 Cyrill Gorcunov 2007-07-21 2232 *elen = le32_to_cpu(sad->extLength) & UDF_EXTENT_LENGTH_MASK;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2233 break;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2234 case ICBTAG_FLAG_AD_LONG:
> 4b11111aba6c80 Marcin Slusarz 2008-02-08 2235 lad = udf_get_filelongad(ptr, alen, &epos->offset, inc);
> 4b11111aba6c80 Marcin Slusarz 2008-02-08 2236 if (!lad)
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2237 return -1;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2238 etype = le32_to_cpu(lad->extLength) >> 30;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2239 *eloc = lelb_to_cpu(lad->extLocation);
> 28de7948a89676 Cyrill Gorcunov 2007-07-21 2240 *elen = le32_to_cpu(lad->extLength) & UDF_EXTENT_LENGTH_MASK;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2241 break;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2242 default:
> fcbf7637e6647e Steve Magnani 2017-10-12 2243 udf_debug("alloc_type = %u unsupported\n", iinfo->i_alloc_type);
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2244 return -1;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2245 }
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2246
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2247 return etype;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2248 }
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 2249
>
Thanks for your time,
--Gian
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] udf: fix uninit-value use in udf_get_fileshortad
2024-09-23 9:23 ` Gianfranco Trad
@ 2024-09-23 14:22 ` Jan Kara
0 siblings, 0 replies; 3+ messages in thread
From: Jan Kara @ 2024-09-23 14:22 UTC (permalink / raw)
To: Gianfranco Trad
Cc: kernel test robot, oe-kbuild, Dan Carpenter, jack, linux-kernel,
syzbot+8901c4560b7ab5c2f9df
On Mon 23-09-24 11:23:48, Gianfranco Trad wrote:
> Hello,
> On 23/09/24 02:26, kernel test robot wrote:
> > BCC: lkp@intel.com
> > CC: oe-kbuild-all@lists.linux.dev
> > In-Reply-To: <20240919195227.412583-1-gianf.trad@gmail.com>
> > References: <20240919195227.412583-1-gianf.trad@gmail.com>
> > TO: Gianfranco Trad <gianf.trad@gmail.com>
> > TO: jack@suse.com
> > CC: linux-kernel@vger.kernel.org
> > CC: skhan@linuxfoundation.org
> > CC: Gianfranco Trad <gianf.trad@gmail.com>
> > CC: syzbot+8901c4560b7ab5c2f9df@syzkaller.appspotmail.com
> >
> > Hi Gianfranco,
> >
> > kernel test robot noticed the following build warnings:
> >
> > [auto build test WARNING on brauner-vfs/vfs.all]
> > [also build test WARNING on linus/master v6.11 next-20240920]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url: https://github.com/intel-lab-lkp/linux/commits/Gianfranco-Trad/udf-fix-uninit-value-use-in-udf_get_fileshortad/20240920-035519
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
> > patch link: https://lore.kernel.org/r/20240919195227.412583-1-gianf.trad%40gmail.com
> > patch subject: [PATCH] udf: fix uninit-value use in udf_get_fileshortad
> > :::::: branch date: 3 days ago
> > :::::: commit date: 3 days ago
> > config: mips-randconfig-r073-20240922 (https://download.01.org/0day-ci/archive/20240923/202409230809.5ozgjLm9-lkp@intel.com/config)
> > compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Reported-by: Dan Carpenter <error27@gmail.com>
> > | Closes: https://lore.kernel.org/r/202409230809.5ozgjLm9-lkp@intel.com/
> >
> > smatch warnings:
> > fs/udf/inode.c:2223 udf_current_aext() error: we previously assumed 'epos->bh' could be null (see line 2204)
> Given this error my initial guess is to update the patch to check the
> assumption of epos->bh not null.
Yes, epos->bh can definitely be NULL here. Generally the check looks really
suspicious. I was trying to find your original patch but for some reason
neither LKML archive nor my mailbox have it. What is the motivation of the
added check? Are you checking for overflow on alen? In that case it would
be best to just use check_add_overflow() helper when computing alen few
lines above...
Honza
> > vim +2223 fs/udf/inode.c
> >
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2193
> > ff116fc8d1d439 Jan Kara 2007-05-08 2194 int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
> > 5ca4e4be841e38 Pekka Enberg 2008-10-15 2195 struct kernel_lb_addr *eloc, uint32_t *elen, int inc)
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2196 {
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2197 int alen;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2198 int8_t etype;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2199 uint8_t *ptr;
> > 5ca4e4be841e38 Pekka Enberg 2008-10-15 2200 struct short_ad *sad;
> > 5ca4e4be841e38 Pekka Enberg 2008-10-15 2201 struct long_ad *lad;
> > 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2202 struct udf_inode_info *iinfo = UDF_I(inode);
> > 28de7948a89676 Cyrill Gorcunov 2007-07-21 2203
> > cb00ea3528eb3c Cyrill Gorcunov 2007-07-19 @2204 if (!epos->bh) {
> > ff116fc8d1d439 Jan Kara 2007-05-08 2205 if (!epos->offset)
> > ff116fc8d1d439 Jan Kara 2007-05-08 2206 epos->offset = udf_file_entry_alloc_offset(inode);
> > 382a2287bf9cd2 Jan Kara 2020-09-25 2207 ptr = iinfo->i_data + epos->offset -
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2208 udf_file_entry_alloc_offset(inode) +
> > 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2209 iinfo->i_lenEAttr;
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2210 alen = udf_file_entry_alloc_offset(inode) +
> > 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2211 iinfo->i_lenAlloc;
> > cb00ea3528eb3c Cyrill Gorcunov 2007-07-19 2212 } else {
> > ff116fc8d1d439 Jan Kara 2007-05-08 2213 if (!epos->offset)
> > ff116fc8d1d439 Jan Kara 2007-05-08 2214 epos->offset = sizeof(struct allocExtDesc);
> > ff116fc8d1d439 Jan Kara 2007-05-08 2215 ptr = epos->bh->b_data + epos->offset;
> > 28de7948a89676 Cyrill Gorcunov 2007-07-21 2216 alen = sizeof(struct allocExtDesc) +
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2217 le32_to_cpu(((struct allocExtDesc *)epos->bh->b_data)->
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2218 lengthAllocDescs);
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2219 }
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2220
> > 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2221 switch (iinfo->i_alloc_type) {
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2222 case ICBTAG_FLAG_AD_SHORT:
> > 2aa242080b8dda Gianfranco Trad 2024-09-19 @2223 if (unlikely(alen < 0 && epos->offset == epos->bh->b_size))
> > 2aa242080b8dda Gianfranco Trad 2024-09-19 2224 return -1;
> The check would be inserted up here in the if clause. But if the patch
> doesn't look good, I can redesign it in a better way. If so, I'm more than
> glad to follow your advice.
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2225 sad = udf_get_fileshortad(ptr, alen, &epos->offset, inc);
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2226 if (!sad)
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2227 return -1;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2228 etype = le32_to_cpu(sad->extLength) >> 30;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2229 eloc->logicalBlockNum = le32_to_cpu(sad->extPosition);
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2230 eloc->partitionReferenceNum =
> > 48d6d8ff7dca80 Marcin Slusarz 2008-02-08 2231 iinfo->i_location.partitionReferenceNum;
> > 28de7948a89676 Cyrill Gorcunov 2007-07-21 2232 *elen = le32_to_cpu(sad->extLength) & UDF_EXTENT_LENGTH_MASK;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2233 break;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2234 case ICBTAG_FLAG_AD_LONG:
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2235 lad = udf_get_filelongad(ptr, alen, &epos->offset, inc);
> > 4b11111aba6c80 Marcin Slusarz 2008-02-08 2236 if (!lad)
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2237 return -1;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2238 etype = le32_to_cpu(lad->extLength) >> 30;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2239 *eloc = lelb_to_cpu(lad->extLocation);
> > 28de7948a89676 Cyrill Gorcunov 2007-07-21 2240 *elen = le32_to_cpu(lad->extLength) & UDF_EXTENT_LENGTH_MASK;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2241 break;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2242 default:
> > fcbf7637e6647e Steve Magnani 2017-10-12 2243 udf_debug("alloc_type = %u unsupported\n", iinfo->i_alloc_type);
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2244 return -1;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2245 }
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2246
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2247 return etype;
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2248 }
> > ^1da177e4c3f41 Linus Torvalds 2005-04-16 2249
> >
> Thanks for your time,
>
> --Gian
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-09-23 14:22 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-09-19 19:52 [PATCH] udf: fix uninit-value use in udf_get_fileshortad Gianfranco Trad
[not found] <202409230809.5ozgjLm9-lkp@intel.com>
2024-09-23 9:23 ` Gianfranco Trad
2024-09-23 14:22 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®