* [PATCH][next] afs: fix integer overflow when shifting 1 more than 32 places
@ 2018-04-11 13:26 Colin King
2018-04-11 13:39 ` David Howells
0 siblings, 1 reply; 7+ messages in thread
From: Colin King @ 2018-04-11 13:26 UTC (permalink / raw)
To: David Howells, linux-afs; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Shifting 1 (a 32 bit signed int) more than 32 places will overflow
the int, so explicitly use 1ULL to avoid this overflow.
Detected by CoverityScan, CID#1467808 ("Uninitentional integer overflow")
Fixes: 63a4681ff39c ("afs: Locally edit directory data for mkdir/create/unlink/...")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
fs/afs/dir_edit.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/afs/dir_edit.c b/fs/afs/dir_edit.c
index 8b400f5aead5..42a63f9467c5 100644
--- a/fs/afs/dir_edit.c
+++ b/fs/afs/dir_edit.c
@@ -40,7 +40,7 @@ static int afs_find_contig_bits(union afs_xdr_dir_block *block, unsigned int nr_
bitmap |= (u64)block->hdr.bitmap[7] << 7 * 8;
bitmap >>= 1; /* The first entry is metadata */
bit = 1;
- mask = (1 << nr_slots) - 1;
+ mask = (1ULL << nr_slots) - 1;
do {
if (sizeof(unsigned long) == 8)
@@ -74,7 +74,7 @@ static void afs_set_contig_bits(union afs_xdr_dir_block *block,
{
u64 mask, before, after;
- mask = (1 << nr_slots) - 1;
+ mask = (1ULL << nr_slots) - 1;
mask <<= bit;
before = *(u64 *)block->hdr.bitmap;
@@ -99,7 +99,7 @@ static void afs_clear_contig_bits(union afs_xdr_dir_block *block,
{
u64 mask, before, after;
- mask = (1 << nr_slots) - 1;
+ mask = (1ULL << nr_slots) - 1;
mask <<= bit;
before = *(u64 *)block->hdr.bitmap;
--
2.17.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH][next] afs: fix integer overflow when shifting 1 more than 32 places
2018-04-11 13:26 [PATCH][next] afs: fix integer overflow when shifting 1 more than 32 places Colin King
@ 2018-04-11 13:39 ` David Howells
2018-04-11 13:42 ` Colin Ian King
2018-04-11 14:10 ` David Howells
0 siblings, 2 replies; 7+ messages in thread
From: David Howells @ 2018-04-11 13:39 UTC (permalink / raw)
To: Colin King; +Cc: dhowells, linux-afs, kernel-janitors, linux-kernel
Colin King <colin.king@canonical.com> wrote:
> Shifting 1 (a 32 bit signed int) more than 32 places will overflow
> the int, so explicitly use 1ULL to avoid this overflow.
> ...
> - mask = (1 << nr_slots) - 1;
> + mask = (1ULL << nr_slots) - 1;
nr_slots cannot be larger than 9, so what I wrote is actually fine and is more
efficient on a 32-bit machine.
David
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] afs: fix integer overflow when shifting 1 more than 32 places
2018-04-11 13:39 ` David Howells
@ 2018-04-11 13:42 ` Colin Ian King
2018-04-11 14:10 ` David Howells
1 sibling, 0 replies; 7+ messages in thread
From: Colin Ian King @ 2018-04-11 13:42 UTC (permalink / raw)
To: David Howells; +Cc: linux-afs, kernel-janitors, linux-kernel
On 11/04/18 14:39, David Howells wrote:
> Colin King <colin.king@canonical.com> wrote:
>
>> Shifting 1 (a 32 bit signed int) more than 32 places will overflow
>> the int, so explicitly use 1ULL to avoid this overflow.
>> ...
>> - mask = (1 << nr_slots) - 1;
>> + mask = (1ULL << nr_slots) - 1;
>
> nr_slots cannot be larger than 9, so what I wrote is actually fine and is more
> efficient on a 32-bit machine.
ok, sorry about the noise.
>
> David
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] afs: fix integer overflow when shifting 1 more than 32 places
2018-04-11 13:39 ` David Howells
2018-04-11 13:42 ` Colin Ian King
@ 2018-04-11 14:10 ` David Howells
2018-04-11 14:17 ` Colin Ian King
` (2 more replies)
1 sibling, 3 replies; 7+ messages in thread
From: David Howells @ 2018-04-11 14:10 UTC (permalink / raw)
To: Colin Ian King; +Cc: dhowells, linux-afs, kernel-janitors, linux-kernel
Colin Ian King <colin.king@canonical.com> wrote:
> >> - mask = (1 << nr_slots) - 1;
> >> + mask = (1ULL << nr_slots) - 1;
> >
> > nr_slots cannot be larger than 9, so what I wrote is actually fine and is
> > more efficient on a 32-bit machine.
>
> ok, sorry about the noise.
It would be possible to cast the value to u64 before assigning it, I suppose.
Would that help? E.g.:
mask = (u64)((1 << nr_slots) - 1);
It looks a bit odd, though, since the cast is made implicitly anyway.
David
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] afs: fix integer overflow when shifting 1 more than 32 places
2018-04-11 14:10 ` David Howells
@ 2018-04-11 14:17 ` Colin Ian King
2018-04-11 15:58 ` Dan Carpenter
2018-04-11 16:03 ` David Howells
2 siblings, 0 replies; 7+ messages in thread
From: Colin Ian King @ 2018-04-11 14:17 UTC (permalink / raw)
To: David Howells; +Cc: linux-afs, kernel-janitors, linux-kernel
On 11/04/18 15:10, David Howells wrote:
> Colin Ian King <colin.king@canonical.com> wrote:
>
>>>> - mask = (1 << nr_slots) - 1;
>>>> + mask = (1ULL << nr_slots) - 1;
>>>
>>> nr_slots cannot be larger than 9, so what I wrote is actually fine and is
>>> more efficient on a 32-bit machine.
>>
>> ok, sorry about the noise.
>
> It would be possible to cast the value to u64 before assigning it, I suppose.
> Would that help? E.g.:
>
> mask = (u64)((1 << nr_slots) - 1);
>
> It looks a bit odd, though, since the cast is made implicitly anyway.
I'm not sure that actually helps, 1 << nr_slots is still evaluating as
a 32 bit value, so that final cast does not may any difference. Anyhow,
since nr_slots is less than 32 then keeping it the way it was is fine.
>
> David
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] afs: fix integer overflow when shifting 1 more than 32 places
2018-04-11 14:10 ` David Howells
2018-04-11 14:17 ` Colin Ian King
@ 2018-04-11 15:58 ` Dan Carpenter
2018-04-11 16:03 ` David Howells
2 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2018-04-11 15:58 UTC (permalink / raw)
To: David Howells; +Cc: Colin Ian King, linux-afs, kernel-janitors, linux-kernel
On Wed, Apr 11, 2018 at 03:10:16PM +0100, David Howells wrote:
> Colin Ian King <colin.king@canonical.com> wrote:
>
> > >> - mask = (1 << nr_slots) - 1;
> > >> + mask = (1ULL << nr_slots) - 1;
> > >
> > > nr_slots cannot be larger than 9, so what I wrote is actually fine and is
> > > more efficient on a 32-bit machine.
> >
> > ok, sorry about the noise.
>
> It would be possible to cast the value to u64 before assigning it, I suppose.
> Would that help? E.g.:
>
> mask = (u64)((1 << nr_slots) - 1);
>
> It looks a bit odd, though, since the cast is made implicitly anyway.
My feeling is that makes it worse. It would introduce a secret,
unpublished static checker warning on my build and it doesn't help me as
a reviewer.
Ideally static analyzers should know that nr_slots is 0-9, but right now
that seems pretty tricky to figure out...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] afs: fix integer overflow when shifting 1 more than 32 places
2018-04-11 14:10 ` David Howells
2018-04-11 14:17 ` Colin Ian King
2018-04-11 15:58 ` Dan Carpenter
@ 2018-04-11 16:03 ` David Howells
2 siblings, 0 replies; 7+ messages in thread
From: David Howells @ 2018-04-11 16:03 UTC (permalink / raw)
To: Dan Carpenter
Cc: dhowells, Colin Ian King, linux-afs, kernel-janitors, linux-kernel
Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Ideally static analyzers should know that nr_slots is 0-9, but right now
> that seems pretty tricky to figure out...
1-9, actually, but, yeah. Maybe the addition of "assertions" that aren't
actually evaluated at runtime?
David
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-04-11 16:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-11 13:26 [PATCH][next] afs: fix integer overflow when shifting 1 more than 32 places Colin King
2018-04-11 13:39 ` David Howells
2018-04-11 13:42 ` Colin Ian King
2018-04-11 14:10 ` David Howells
2018-04-11 14:17 ` Colin Ian King
2018-04-11 15:58 ` Dan Carpenter
2018-04-11 16:03 ` David Howells
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).