* [bug report] fs/ntfs3: Add attrib operations
@ 2021-08-24 9:53 Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-08-24 9:53 UTC (permalink / raw)
To: almaz.alexandrovich; +Cc: ntfs3
Hello Konstantin Komarov,
The patch be71b5cba2e6: "fs/ntfs3: Add attrib operations" from Aug
13, 2021, leads to the following
Smatch static checker warning:
fs/ntfs3/attrib.c:882 attr_data_get_block()
warn: was expecting a 64 bit value instead of '~(clst_per_frame - 1)'
fs/ntfs3/attrib.c
823 int attr_data_get_block(struct ntfs_inode *ni, CLST vcn, CLST clen, CLST *lcn,
824 CLST *len, bool *new)
825 {
826 int err = 0;
827 struct runs_tree *run = &ni->file.run;
828 struct ntfs_sb_info *sbi;
829 u8 cluster_bits;
830 struct ATTRIB *attr = NULL, *attr_b;
831 struct ATTR_LIST_ENTRY *le, *le_b;
832 struct mft_inode *mi, *mi_b;
833 CLST hint, svcn, to_alloc, evcn1, next_svcn, asize, end;
834 u64 total_size;
835 u32 clst_per_frame;
836 bool ok;
837
838 if (new)
839 *new = false;
840
841 down_read(&ni->file.run_lock);
842 ok = run_lookup_entry(run, vcn, lcn, len, NULL);
843 up_read(&ni->file.run_lock);
844
845 if (ok && (*lcn != SPARSE_LCN || !new)) {
846 /* normal way */
847 return 0;
848 }
849
850 if (!clen)
851 clen = 1;
852
853 if (ok && clen > *len)
854 clen = *len;
855
856 sbi = ni->mi.sbi;
857 cluster_bits = sbi->cluster_bits;
858
859 ni_lock(ni);
860 down_write(&ni->file.run_lock);
861
862 le_b = NULL;
863 attr_b = ni_find_attr(ni, NULL, &le_b, ATTR_DATA, NULL, 0, NULL, &mi_b);
864 if (!attr_b) {
865 err = -ENOENT;
866 goto out;
867 }
868
869 if (!attr_b->non_res) {
870 *lcn = RESIDENT_LCN;
871 *len = 1;
872 goto out;
873 }
874
875 asize = le64_to_cpu(attr_b->nres.alloc_size) >> sbi->cluster_bits;
876 if (vcn >= asize) {
877 err = -EINVAL;
878 goto out;
879 }
880
881 clst_per_frame = 1u << attr_b->nres.c_unit;
--> 882 to_alloc = (clen + clst_per_frame - 1) & ~(clst_per_frame - 1);
In this config "clen" is a u64 and "clst_per_frame" is a u32 so this
code will truncate to_alloc to a u32. An easy fix is to use the
ALIGN() macro.
to_alloc = ALIGN(clen, clst_per_frame);
However, I still had some questions below so I did not write a patch.
883
884 if (vcn + to_alloc > asize)
885 to_alloc = asize - vcn;
If to_alloc is too large for asize then make it smaller. Is it still
ALIGNED?
886
887 svcn = le64_to_cpu(attr_b->nres.svcn);
888 evcn1 = le64_to_cpu(attr_b->nres.evcn) + 1;
889
890 attr = attr_b;
891 le = le_b;
892 mi = mi_b;
893
894 if (le_b && (vcn < svcn || evcn1 <= vcn)) {
895 attr = ni_find_attr(ni, attr_b, &le, ATTR_DATA, NULL, 0, &vcn,
896 &mi);
897 if (!attr) {
898 err = -EINVAL;
899 goto out;
900 }
901 svcn = le64_to_cpu(attr->nres.svcn);
902 evcn1 = le64_to_cpu(attr->nres.evcn) + 1;
903 }
904
905 err = attr_load_runs(attr, ni, run, NULL);
906 if (err)
907 goto out;
908
909 if (!ok) {
910 ok = run_lookup_entry(run, vcn, lcn, len, NULL);
911 if (ok && (*lcn != SPARSE_LCN || !new)) {
912 /* normal way */
913 err = 0;
914 goto ok;
915 }
916
917 if (!ok && !new) {
918 *len = 0;
919 err = 0;
920 goto ok;
921 }
922
923 if (ok && clen > *len) {
924 clen = *len;
925 to_alloc = (clen + clst_per_frame - 1) &
926 ~(clst_per_frame - 1);
We re-assign to_alloc here. And it's smaller than before, but it hasn't
been checked against asize so it might not be small enough?
927 }
928 }
929
930 if (!is_attr_ext(attr_b)) {
931 err = -EINVAL;
932 goto out;
933 }
934
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* [bug report] fs/ntfs3: Add attrib operations
@ 2026-04-10 10:15 Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2026-04-10 10:15 UTC (permalink / raw)
To: Konstantin Komarov; +Cc: ntfs3
Hello Konstantin Komarov,
Commit be71b5cba2e6 ("fs/ntfs3: Add attrib operations") from Aug 13,
2021 (linux-next), leads to the following Smatch static checker
warning:
fs/ntfs3/attrib.c:176 attr_allocate_clusters()
error: we previously assumed 'pre_alloc' could be null (see line 167)
fs/ntfs3/attrib.c
160 int attr_allocate_clusters(struct ntfs_sb_info *sbi, struct runs_tree *run,
161 struct runs_tree *run_da, CLST vcn, CLST lcn,
162 CLST len, CLST *pre_alloc, enum ALLOCATE_OPT opt,
163 CLST *alen, const size_t fr, CLST *new_lcn,
164 CLST *new_len)
165 {
166 int err;
167 CLST flen, vcn0 = vcn, pre = pre_alloc ? *pre_alloc : 0;
168 size_t cnt = run->count;
169
170 for (;;) {
171 err = ntfs_look_for_free_space(sbi, lcn, len + pre, &lcn, &flen,
172 opt);
173
174 if (err == -ENOSPC && pre) {
175 pre = 0;
--> 176 if (*pre_alloc)
I'm not sure why warnings from 2021 are showing up as new warnings but
presumably this should be:
if (pre_alloc)
*pre_alloc = 0;
177 *pre_alloc = 0;
178 continue;
179 }
180
181 if (err == -ENOSPC && new_len && vcn - vcn0) {
182 /* Keep already allocated clusters. */
183 *alen = vcn - vcn0;
This email is a free service from the Smatch-CI project [smatch.sf.net].
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* [bug report] fs/ntfs3: Add attrib operations
@ 2023-07-25 11:45 Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2023-07-25 11:45 UTC (permalink / raw)
To: almaz.alexandrovich; +Cc: ntfs3
Hello Konstantin Komarov,
The patch be71b5cba2e6: "fs/ntfs3: Add attrib operations" from Aug
13, 2021 (linux-next), leads to the following Smatch static checker
warning:
fs/ntfs3/xattr.c:393 ntfs_set_ea()
warn: integer overflows
fs/ntfs3/xattr.c
301 static noinline int ntfs_set_ea(struct inode *inode, const char *name,
302 size_t name_len, const void *value,
303 size_t val_size, int flags, bool locked,
304 __le16 *ea_size)
305 {
306 struct ntfs_inode *ni = ntfs_i(inode);
307 struct ntfs_sb_info *sbi = ni->mi.sbi;
308 int err;
309 struct EA_INFO ea_info;
310 const struct EA_INFO *info;
311 struct EA_FULL *new_ea;
312 struct EA_FULL *ea_all = NULL;
313 size_t add, new_pack;
314 u32 off, size, ea_sz;
315 __le16 size_pack;
316 struct ATTRIB *attr;
317 struct ATTR_LIST_ENTRY *le;
318 struct mft_inode *mi;
319 struct runs_tree ea_run;
320 u64 new_sz;
321 void *p;
322
323 if (!locked)
324 ni_lock(ni);
325
326 run_init(&ea_run);
327
328 if (name_len > 255) {
329 err = -ENAMETOOLONG;
330 goto out;
331 }
332
333 add = ALIGN(struct_size(ea_all, name, 1 + name_len + val_size), 4);
It's bad to mix struct_size() with any sort of math. Going into it, can
this overflow "1 + name_len + val_size"? And then struct_size() returns
ULONG_MAX if there is an overflow. When you pass that to ALIGN() it
becomes zero.
334
335 err = ntfs_read_ea(ni, &ea_all, add, &info);
336 if (err)
337 goto out;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* [bug report] fs/ntfs3: Add attrib operations
@ 2021-08-24 9:42 Dan Carpenter
2021-08-24 10:49 ` Kari Argillander
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2021-08-24 9:42 UTC (permalink / raw)
To: almaz.alexandrovich; +Cc: ntfs3
Hello Konstantin Komarov,
The patch be71b5cba2e6: "fs/ntfs3: Add attrib operations" from Aug
13, 2021, leads to the following
Smatch static checker warning:
fs/ntfs3/attrib.c:383 attr_set_size_res()
warn: was expecting a 64 bit value instead of '~7'
fs/ntfs3/attrib.c
370 static int attr_set_size_res(struct ntfs_inode *ni, struct ATTRIB *attr,
371 struct ATTR_LIST_ENTRY *le, struct mft_inode *mi,
372 u64 new_size, struct runs_tree *run,
373 struct ATTRIB **ins_attr)
374 {
375 struct ntfs_sb_info *sbi = mi->sbi;
376 struct MFT_REC *rec = mi->mrec;
377 u32 used = le32_to_cpu(rec->used);
378 u32 asize = le32_to_cpu(attr->size);
379 u32 aoff = PtrOffset(rec, attr);
380 u32 rsize = le32_to_cpu(attr->res.data_size);
381 u32 tail = used - aoff - asize;
382 char *next = Add2Ptr(attr, asize);
--> 383 s64 dsize = QuadAlign(new_size) - QuadAlign(rsize);
^^^^^^^^^^^^^^^^^^
QuadAlign() is a bad name.
The ntfs3 code has a bunch of bad macros like ntfs_malloc() which will
need to be removed hopefully? I haven't seen this code before today but
presumably everyone has mentioned this already.
Anyway, new_size is a u64 and QuadAlign() truncates it to u32. Use the
normal ALIGN() macro.
384
385 if (dsize < 0) {
386 memmove(next + dsize, next, tail);
387 } else if (dsize > 0) {
388 if (used + dsize > sbi->max_bytes_per_attr)
389 return attr_make_nonresident(ni, attr, le, mi, new_size,
390 run, ins_attr, NULL);
391
392 memmove(next + dsize, next, tail);
393 memset(next, 0, dsize);
394 }
395
396 if (new_size > rsize)
397 memset(Add2Ptr(resident_data(attr), rsize), 0,
398 new_size - rsize);
399
400 rec->used = cpu_to_le32(used + dsize);
401 attr->size = cpu_to_le32(asize + dsize);
402 attr->res.data_size = cpu_to_le32(new_size);
403 mi->dirty = true;
404 *ins_attr = attr;
405
406 return 0;
407 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] fs/ntfs3: Add attrib operations
2021-08-24 9:42 Dan Carpenter
@ 2021-08-24 10:49 ` Kari Argillander
0 siblings, 0 replies; 5+ messages in thread
From: Kari Argillander @ 2021-08-24 10:49 UTC (permalink / raw)
To: Dan Carpenter; +Cc: almaz.alexandrovich, ntfs3
On Tue, Aug 24, 2021 at 12:42:49PM +0300, Dan Carpenter wrote:
> Hello Konstantin Komarov,
>
> The patch be71b5cba2e6: "fs/ntfs3: Add attrib operations" from Aug
> 13, 2021, leads to the following
> Smatch static checker warning:
>
> fs/ntfs3/attrib.c:383 attr_set_size_res()
> warn: was expecting a 64 bit value instead of '~7'
>
> fs/ntfs3/attrib.c
> 370 static int attr_set_size_res(struct ntfs_inode *ni, struct ATTRIB *attr,
> 371 struct ATTR_LIST_ENTRY *le, struct mft_inode *mi,
> 372 u64 new_size, struct runs_tree *run,
> 373 struct ATTRIB **ins_attr)
> 374 {
> 375 struct ntfs_sb_info *sbi = mi->sbi;
> 376 struct MFT_REC *rec = mi->mrec;
> 377 u32 used = le32_to_cpu(rec->used);
> 378 u32 asize = le32_to_cpu(attr->size);
> 379 u32 aoff = PtrOffset(rec, attr);
> 380 u32 rsize = le32_to_cpu(attr->res.data_size);
> 381 u32 tail = used - aoff - asize;
> 382 char *next = Add2Ptr(attr, asize);
> --> 383 s64 dsize = QuadAlign(new_size) - QuadAlign(rsize);
> ^^^^^^^^^^^^^^^^^^
> QuadAlign() is a bad name.
Agreed. I will make patch to this someday if nobody else haven't done it
already by then. These kind of macros can confuse checkpatch and
coccinelle so that is one good reason to get away from them.
>
> The ntfs3 code has a bunch of bad macros like ntfs_malloc() which will
> need to be removed hopefully? I haven't seen this code before today but
> presumably everyone has mentioned this already.
ntfs_malloc has already been addressed and patch is made.
lore.kernel.org/ntfs3/20210818010649.412912-1-kari.argillander@gmail.com/
>
> Anyway, new_size is a u64 and QuadAlign() truncates it to u32. Use the
> normal ALIGN() macro.
>
> 384
> 385 if (dsize < 0) {
> 386 memmove(next + dsize, next, tail);
> 387 } else if (dsize > 0) {
> 388 if (used + dsize > sbi->max_bytes_per_attr)
> 389 return attr_make_nonresident(ni, attr, le, mi, new_size,
> 390 run, ins_attr, NULL);
> 391
> 392 memmove(next + dsize, next, tail);
> 393 memset(next, 0, dsize);
> 394 }
> 395
> 396 if (new_size > rsize)
> 397 memset(Add2Ptr(resident_data(attr), rsize), 0,
> 398 new_size - rsize);
> 399
> 400 rec->used = cpu_to_le32(used + dsize);
> 401 attr->size = cpu_to_le32(asize + dsize);
> 402 attr->res.data_size = cpu_to_le32(new_size);
> 403 mi->dirty = true;
> 404 *ins_attr = attr;
> 405
> 406 return 0;
> 407 }
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-04-10 10:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-24 9:53 [bug report] fs/ntfs3: Add attrib operations Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2026-04-10 10:15 Dan Carpenter
2023-07-25 11:45 Dan Carpenter
2021-08-24 9:42 Dan Carpenter
2021-08-24 10:49 ` Kari Argillander
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox